Add support for OCSP #1160
Add support for OCSP #1160
Conversation
cmd/internal/cli/x509.go
Outdated
| // LICENSE.md file distributed with the sources of this project regarding your | ||
| // rights to use or distribute this software. | ||
|
|
||
| package cli |
There was a problem hiding this comment.
If possible, this implementation should not be in the cmd/internal/cli package but somewhere appropriate under internal/pkg.
We're trying to move implementation logic out of the cli package, so that it really just has flag / option handling etc.
There was a problem hiding this comment.
I see. It shouldn't be a problem.
cmd/internal/cli/verify.go
Outdated
| // If the certificate is self-signed certificate, we can use the certificate itself as one of the roots. | ||
| // Otherwise, we need the certificate of the authorities who signed the certificate. | ||
| var certificateIsSelfSigned bool | ||
|
|
||
| if string(cert.AuthorityKeyId) == string(cert.SubjectKeyId) { | ||
| selfRoot := x509.NewCertPool() | ||
| selfRoot.AddCert(cert) | ||
|
|
||
| opts = append(opts, singularity.OptVerifyWithRoots(selfRoot)) | ||
| certificateIsSelfSigned = true | ||
| } | ||
|
|
||
| intermediateCertificates := certPool{} | ||
| rootCertificates := certPool{} | ||
|
|
||
| if cmd.Flag(verifyCertificateIntermediatesFlag.Name).Changed { | ||
| if certificateIsSelfSigned { | ||
| sylog.Infof("Ignore intermediate certificates due to the certificate being self-signed.") | ||
| } else { | ||
| intermediates, err := loadCertificatePool(certificateIntermediatesPath) | ||
| if err != nil { | ||
| sylog.Fatalf("Failed to load intermediate certificates: %v", err) | ||
| } | ||
|
|
||
| intermediateCertificates = intermediates | ||
| opts = append(opts, singularity.OptVerifyWithIntermediates(intermediates.x509CertPool())) | ||
| } | ||
| } | ||
|
|
||
| if cmd.Flag(verifyCertificateRootsFlag.Name).Changed { | ||
| if certificateIsSelfSigned { | ||
| sylog.Infof("Ignore root certificates due to the certificate being self-signed.") | ||
| } else { | ||
| roots, err := loadCertificatePool(certificateRootsPath) | ||
| if err != nil { | ||
| sylog.Fatalf("Failed to load root certificates: %v", err) | ||
| } | ||
|
|
||
| rootCertificates = roots | ||
| opts = append(opts, singularity.OptVerifyWithRoots(roots.x509CertPool())) | ||
| } | ||
| } | ||
|
|
||
| if cmd.Flag(verifyOCSPFlag.Name).Changed { | ||
| if certificateIsSelfSigned { | ||
| sylog.Infof("Ignore OCSP due to the certificate being self-signed.") | ||
| } else { | ||
| ocspErr := OnlineRevocationCheck(cert, intermediateCertificates, rootCertificates) | ||
| if errors.Is(ocspErr, errOCSPQuery) { | ||
| // TODO: We need to decide whether this should be strict or permissive. | ||
| sylog.Fatalf("OCSP server is unavailable: %v", ocspErr) | ||
| } else if ocspErr != nil { | ||
| sylog.Fatalf("OCSP verification has failed: %v", ocspErr) | ||
| } | ||
|
|
||
| sylog.Infof("OCSP validation has passed") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The cmd/internal/cli package shouldn't contain any logic aside from loading the certificates/pools specified on the command line and passing them on to the application code (internal/app/singularity.) The idea being that non-CLI code could just as easily need to verify a signature, and the behaviour shouldn't differ whether it's the user or Singularity itself calling the code.
In the context of adding online verification, this package should simply be appending to opts a functional (singularity.OptVerifyWithOCSP?) option defined in internal/app/singularity. Does that make sense?
cmd/internal/cli/verify.go
Outdated
|
|
||
| // If the certificate is self-signed certificate, we can use the certificate itself as one of the roots. | ||
| // Otherwise, we need the certificate of the authorities who signed the certificate. | ||
| var certificateIsSelfSigned bool |
There was a problem hiding this comment.
I think it would be wise to tackle any self-signed certificate functionality separate from the online verification work. We probably need to discuss what should happen when a self-signed certificate is specified. My gut instinct is that Singularity should either:
- Not accept a self-signed cert unless it is present in the system pool, or
- Accept it only if the user has specified this as the desired behaviour.
As an example of how this is handled elsewhere, wget --no-check-certificate allows a self-signed cert to be used. That being said, its documentation actually recommends the first approach for self-signed certs:
For self-signed/internal certificates, you should download the
certificate and verify against that instead of forcing this insecure mode.
cmd/internal/cli/x509.go
Outdated
| // certPool is needed because *x509.x509CertPool does not support listing of Certificates. | ||
| // Without listing, it is impossible to perform Revocation Checking. | ||
| type certPool map[string]*x509.Certificate |
There was a problem hiding this comment.
I'm not sure I understand why we can't do revocation listing without defining this type?
The logic in internal/app/singularity is calling Certificate.Verify (here), which will return one or more certificate chains based on the certificate and pool(s) it is passed.
When --ocsp-verify is specified, I think what we'll need to do is add on to that by walk the certificate chain(s) and do OCSP verification (call queryOCSP) on each certificate in the chain. Does that make sense?
This PR implements the OCSP support discussed in #1095 and #1152.
The reference implementation is based on RFC https://www.rfc-editor.org/rfc/rfc6960
Tests has been based on this doc.
https://akshayranganath.github.io/OCSP-Validation-With-Openssl/
Step 1: Get the server certificate
Step 2: Get the intermediate certificate
Step 3: Run Singularity in OCSP mode
Expected outputs:
If communication error: FATAL: OCSP server is unavailable: certificate 'CN=www.akamai.com,O=Akamai Technologies, Inc.,L=Cambridge,ST=Massachusetts,C=US': failed to contact OCSP (additional information is printed to identify which certificate has failed)
If OCSP OK: FATAL: Failed to verify container: integrity: signature object 5 not valid: dsse: verify envelope failed: Accepted signatures do not match threshold, Found: 0, Expected 1 (reasonable since the certificate is just for testing the OCSP and not for validating the signature).
If use invalid certificates: FATAL: Failed to verify container: x509: certificate signed by unknown authority