Add method to verify certificate chain #2
Conversation
| // Returns the full ordered chain: [leaf, intermediates..., root] | ||
| func buildAndValidateCertChain(certChain []*x509.Certificate) error { | ||
| if len(certChain) < 2 { | ||
| return errors.New("certificate chain must contain at least two certificates") |
There was a problem hiding this comment.
Any reason for rejecting self-signed certificates?
There was a problem hiding this comment.
One of the constraints for a valid chain, specified here under "Other Constraints"
A valid certificate chain MUST contain a minimum of two certificates - a leaf and a root
There was a problem hiding this comment.
@gokarnm @priteshbandi
It is not clear why we are not supporting self-signed certificates, which may be easier for testing purposes.
There was a problem hiding this comment.
@rgnote - I too think the implementation should support self signed certificates. The most common use case will be testing. We should also document ( or link to existing document) how someone can generate a self signed certificate.
| @@ -30,3 +33,35 @@ func ParseCertificatePEM(data []byte) ([]*x509.Certificate, error) { | |||
| } | |||
| return certs, nil | |||
| } | |||
|
|
|||
| // ValidateCertChain takes an ordered certificate chain and validates issuance from leaf to root | |||
| func ValidateCertChain(certChain []*x509.Certificate) error { | |||
There was a problem hiding this comment.
This method only check if the cert chain is correctly formed, not that it chains to an trusted root certificate in trust store correct? Where will we have that logic. Also is this the method where we will add additional cert chain constraint check (KU, EKU, path len)?
We should create an issue to track that work and add TODO with the issue link, if that logic will be included here.
@priteshbandi , any feedback?
There was a problem hiding this comment.
Yes we will have a separate method to check if chain ends in a specific root. Also I think the cert constraint checks belong in a separate method. We already have some of those validations here. I'll track the remaining work, it's not much
| @@ -30,3 +33,35 @@ func ParseCertificatePEM(data []byte) ([]*x509.Certificate, error) { | |||
| } | |||
| return certs, nil | |||
| } | |||
|
|
|||
| // ValidateCertChain takes an ordered certificate chain and validates issuance from leaf to root | |||
| func ValidateCertChain(certChain []*x509.Certificate) error { | |||
There was a problem hiding this comment.
I'd rename this to ValidateSigningCertChain if this is going to be distinct from TSA cert chain validation.
There was a problem hiding this comment.
This method works for general cert validation, tsa or signing chain. I think specific validations should be in separate methods
Signed-off-by: Jonathan Donas <[email protected]>
|
@shizhMSFT @dtzar merging this PR as per the new review criteria. |
Signed-off-by: Jonathan Donas [email protected]