-
Notifications
You must be signed in to change notification settings - Fork 172
pkcs8: add Attributes field to PrivateKeyInfo
#2243
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
Draft
kraemv
wants to merge
1
commit into
RustCrypto:master
Choose a base branch
from
kraemv:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+173
−27
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Oops, something went wrong.
Oops, something went wrong.
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.
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.
If I read the RFC5208 correctly, attributes are defined as:
I believe the Attribute itself comes from X501 (section 8.2):
I believe this is the same structure as the one in x509: https://docs.rs/x509-cert/latest/x509_cert/attr/struct.Attribute.html
Ideally we'd have an AttributeRef<'a> and an Attribute somewhere we could reuse, and attributes would be SetOf::<AttributeRef<'a>>, sadly we don't have that.
But I'd love if we could drop the changes on the der side. I'm not sure I agree with the fallback on Raw bytes really.
Something like:
Maybe?
Uh oh!
There was an error while loading. Please reload this page.
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.
At one point we had an
x501crate to define it: #377I think it would be annoying to add a whole separate dependency to
pkcs8just to provideAttributethough, unless it were optional.I agree this PR seems a bit large solely for accomplishing the stated goal, though a
SetOfRefcould potentially be useful for other purposes, but should probably be added in its own separate PR toderfirst before we go that way.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.
In the superseding RFC 5958 it says:
The attributes field uses the class ATTRIBUTE which is restricted by the OneAsymmetricKeyAttributes information object set. OneAsymmetricKeyAttributes is an open ended set in this document. Others documents can constrain these values.But I agree, that restricting to x509 attributes is a good choice here.
Restricting to AttributeTypeAndValue will not be sufficient, as e.g. OpenSSL parsing routines for PrivateKeyInfo expect an Attribute (with a set of values). Also, the problem with SetOf in the no_std case is, that it always requires a const generic size N, which then spreads over PrivateKeyInfo everywhere.
Should I open up a separate PR to implement SetOfRef first?
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.
Yes, splitting out the
SetOfRefstuff so it can be reviewed separately would be great.I think it's fine to have duplication for now, and if it becomes a problem we can revive the
x501crate (which I guess was removed in #445)