Fix package name and add DER parsing support #3
Conversation
Signed-off-by: rgnote <[email protected]>
x509/cert.go
Outdated
| if err == nil { | ||
| certs = append(certs, cert) | ||
| } |
There was a problem hiding this comment.
Missing else condition where it return an error.
There was a problem hiding this comment.
There is nothing to do in the else condition. This function returns a list of certs that are parsable in the input. If no certs are found, it returns an empty list (not an error).
There was a problem hiding this comment.
I guess if a user had malformed a DER, the error message from ParseCertificate would have been useful. Similar to line 33 where we return an error.
There was a problem hiding this comment.
Good point. Addressed it.
| if err != nil { | ||
| return nil, err | ||
| if block == nil { | ||
| // data may be in DER format |
There was a problem hiding this comment.
| // data may be in DER format | |
| // data may be in DER format, data contains a single DER encoded certificate. |
There was a problem hiding this comment.
Is the intent to parse a single DER cert, should we use x509.ParseCertificates instead? Multiple PEM in a file are common, not sure about DER though, how common it it. Either ways, this code is called to load trust store which should support PEM bundles at least.
There was a problem hiding this comment.
AFAIK, DER bundles are not common. It requires people to follow some conventions. For example, x509.ParseCertificates() works only if the certs are concatenated with no intermediate padding.
That being said, there is no reason to not use x509.ParseCertificates though. I will fix this. We are already handle the PEM bundles.
| func ParseCertificatePEM(data []byte) ([]*x509.Certificate, error) { | ||
| // parseCertificates parses certificates from either PEM or DER data | ||
| // returns an empty list if no certificates are found | ||
| func parseCertificates(data []byte) ([]*x509.Certificate, error) { |
There was a problem hiding this comment.
Can we add test cases for ReadCertificateFile if not present.
There was a problem hiding this comment.
We should add tests here. Some of the existing code in notation-go and core-go is missing test cases. Will address this separately. This code is being tested through trust-store parsing though.
There was a problem hiding this comment.
Sounds good, can you track this with an issue.
There was a problem hiding this comment.
Added tests for cert utilities in this PR
Signed-off-by: rgnote <[email protected]>
Signed-off-by: rgnote [email protected]