Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle PCK certificates #1068
Handle PCK certificates #1068
Changes from 21 commits
44501dc
d27ceba
ca6e09c
11575c4
6834392
a1ae9e4
8ed311a
721bc82
94d563f
6420aa9
a170da5
d0663b7
eda8bf0
e02b0f4
aac90aa
f5941a2
1d6da3a
0c50bbb
55abbd3
7815a87
31eca99
8c15035
198da10
eef45ce
e8d7a78
672a1bf
021aac3
097c188
6f0acd6
689e2f0
4fc7cb2
a6c5cea
841b167
03c6b37
d99fc35
8d57919
beb4e7e
3968d6e
a053e3b
d7feb00
27d64eb
06d3028
944f931
14014a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is given as an argument to the
validate
extrinsic. I don't like having this extra struct, but i couldn't come up with another way of doing it which didn't make it possible that there exists aServerInfo
with no associated PCK which i want to avoid.Ideally i would like to
impl From<ServerInfo> for JoiningServerInfo
but because it needs to call the cert validator which is a trait method, i couldn't figure out how to do that either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you actually want the other way around, but either way ya it seems a little tricky because of the
T
bound. We can do it as an internal method on the pallet implementation though.But really the above conversion doesn't really handle the crux of the problem which is this. One approach is that we can pass in the certificate chain directly to the extrinsic and then have the
ServerInfo.provisioning_certification_key
field be anOption
. It doesn't guarantee that it can't beNone
, but it is something that can be verified before writingServerInfo
to storage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel pretty strongly about wanting assurance at the type level that every
ServerInfo
has a PCK - i don't want it to be anOption
.I would like to somehow improve the conversion between
JoiningServerInfo
andServerInfo
but its going to have to be in a follow up as i don't wanna keep resolving conflicts here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add the error variant for these as
PckCertificate(PckParseVerifyError)
, but i couldn't get adding#[pallet::Error]
to that error type to work, and without it it doesn't implement the needed traits.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try and implement a
From
implementation on these so that we can use?/into()
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 i've added one but we still need to call
map_err
to tell rust what type we are converting into because this returns aDispatchResult
, not apallet::Error<T>
.