fix: Validate SSH certificate that it doesn't contain more privilege than requested#361
fix: Validate SSH certificate that it doesn't contain more privilege than requested#361minhtule wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #361 +/- ##
==========================================
+ Coverage 86.18% 86.80% +0.62%
==========================================
Files 40 40
Lines 2830 2865 +35
==========================================
+ Hits 2439 2487 +48
+ Misses 265 261 -4
+ Partials 126 117 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
minhtule
changed the title
There was a problem hiding this comment.
Pull request overview
This PR strengthens the SSH certificate signing flow by validating that a CA-issued SSH certificate does not exceed the privileges requested by the gateway, aligning with Issue #360’s requirement to distrust potentially misconfigured/compromised external CAs.
Changes:
- Adds
verifyCertificateand enforces it on Vault-signed certificates before use. - Validates certificate type, bound public key, principals, validity, and requested permissions/attributes.
- Adds targeted unit tests covering policy-violation and Vault signing error scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/sshhandler/ca.go | Parses Vault-issued certs and enforces post-signing policy validation via verifyCertificate. |
| internal/sshhandler/ca_test.go | Adds tests for Vault signing error paths and comprehensive verifyCertificate policy checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| maxValidBefore := mustUint64(time.Now().Add(req.ttl).Add(clockSkewBuffer)) | ||
| if cert.ValidBefore > maxValidBefore { | ||
| return fmt.Errorf("%w: validity %d exceeds requested TTL (max %d)", errCertPolicyViolation, cert.ValidBefore, maxValidBefore) | ||
| } |
There was a problem hiding this comment.
The TTL is anchored at when the gateway receives and starts using the cert (it's used immediately on return), not when the request was sent. Treating "valid for ttl from when the cert becomes usable" as the intended meaning is reasonable, and it's also what Vault does (ValidBefore = signing_time + ttl). Under that reading the response delay doesn't grant extra lifetime, so I'll leave the check as-is
… match the request
minhtule
force-pushed
the
feat/mt/validate-vault-ssh-certificate
branch
from
0cf0329 to
e2833f7
Compare
| maxValidBefore := mustUint64(time.Now().Add(req.ttl).Add(clockSkewBuffer)) | ||
| if cert.ValidBefore > maxValidBefore { | ||
| return fmt.Errorf("%w: validity %d exceeds requested TTL (max %d)", errCertPolicyViolation, cert.ValidBefore, maxValidBefore) | ||
| } |
There was a problem hiding this comment.
Improve the cert renewal logic in #362 so backdating ValidAfter doesn't cause any issue. Enforcing a lower bound is kind of arbitrary so not adding this validation.
| {name: "validity exceeds TTL", req: userReq, setupFn: func(c *ssh.Certificate) { | ||
| c.ValidBefore = mustUint64(now.Add(2 * time.Hour)) | ||
| }, wantErrMsg: "exceeds requested TTL"}, | ||
| {name: "no expiry", req: userReq, setupFn: func(c *ssh.Certificate) { | ||
| c.ValidBefore = ssh.CertTimeInfinity | ||
| }, wantErrMsg: "exceeds requested TTL"}, |
There was a problem hiding this comment.
Improve the cert renewal logic in #362 so backdating ValidAfter doesn't cause any issue. Enforcing a lower bound is kind of arbitrary so not adding this validation.
minhtule
force-pushed
the
feat/mt/validate-vault-ssh-certificate
branch
from
d3c348b to
42aefa0
Compare
| require.NotErrorIs(t, err, errVaultSignFailed) | ||
| } | ||
|
|
||
| func TestVerifyCertificate(t *testing.T) { |
There was a problem hiding this comment.
Only this test is needed for the code change. The other tests for Vault CA (which didn't have any test!) is to improve code coverage.
clement0010
left a comment
clement0010
left a comment
There was a problem hiding this comment.
Fix & merge! One question: see below
| for opt := range cert.CriticalOptions { | ||
| if _, ok := req.permissions.CriticalOptions[opt]; !ok { | ||
| return fmt.Errorf("%w: unexpected critical option %q", errCertPolicyViolation, opt) | ||
| } | ||
| } |
There was a problem hiding this comment.
If CriticalOptions is restrictive, having extra critical options should technically be fine? 🤔
There was a problem hiding this comment.
That's true in general but force-command is the exception. It substitutes what runs on the session rather than narrowing it, so a CA injecting it (we currently request zero critical options) would silently change behavior. This is a fail-closed check on the CA doing exactly what we asked, so I'd keep the exact-match and reject anything unrequested.
Related Tickets & Documents
Changes