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
x501+x509:
CertificateDocument
,X509Version
, and more additions #393x501+x509:
CertificateDocument
,X509Version
, and more additions #393Changes from 10 commits
f2ab456
213e9a2
945bfe9
c2d3b40
40cf4aa
7504101
30c77d0
66e6b9a
89dc750
b34f48b
bc391a7
c2f9377
1e44e55
fb203b6
ee9f1a3
c226d37
b600b99
6fe650f
a9dab3e
b7a19de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 spirit of RFC356 I'd suggest simply calling this
Version
.That also follows the convention used by other version types in this repo.
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.
ok. will change as i mop up cargo hack findings.
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.
#410 allows you to derive this type.
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.
@carl-wallace All the changes in this file are obsolete if we merge #410 instead.
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.
Sure. Further review of this until after alignment with changes to dependencies is likely not worthwhile.
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.
Seems like just
self.version
should work here, especially if you make itCopy
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.
Addressed here (initially I though you meant replace that production with self.version but landed as shown): carl-wallace@c226d37
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.
@tarcieri I know I've implemented one of these document types for
pkcs10
. However:What is the purpose of them? I don't understand how they are intended to be used. It seems that PEM decoding is implemented through the document types. However, in order to instantiate a document, you have to fully parse the DER. So why not just have the fully parsed type?
If there is a good reason for the document types, surely these can be genericized. They are all exactly the same code and differ only by the type of the value decoded therein. So
Document<T: Decodable + Encodable>
seems like an obvious win 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.
This seems like a complicated and important enough issue I did a long-form write up on it, so as not to lose the answers to these questions in a PR comment: #421
Some short answers though:
The purpose of these types is discussed at length in der:
*Document
type improvements #421, but in regard to PEM specifically, sinceDecode<'a>
is zero-copy and needs to borrow from a slice containing DER, parsing PEM needs to decode DER into a buffer thatDecode<'a>
can borrow from. If we had something likeDecodeOwned
which didn't need to borrow, documents could be decoded directly from PEM. As it were, this is how thessh-key
crate works, and I just added support for incremental/buffered PEM decoding to thepem-rfc7468
crate (pem-rfc7468: buffered Base64 decoder #406) and changed thessh-key
crate to use that (ssh-key: usepem-rfc7468
for private key PEM parser #407). But that only works because thessh-key
crate is built entirely around owned types (and doesn't use ASN.1 DER).The problem with
Document<T: Decodable + Encodable>
is you omitted the lifetime onDecodable<'a>
. Since it's a'self
borrow and Rust doesn't actually have a way to notate a'self
lifetime you can't notate that on a struct definition, however as I noted in der:*Document
type improvements #421 it can probably be done by moving the bounds to individual methods soDecodable<'a>
can hang offpub fn decode<'a>(&'a self) -> T where T: Decode<'a>