-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Adding support for chained intermediate CAs in pki backend #1694
Changes from 30 commits
8cafd44
150ca81
63b4866
8260062
2cd3859
3e50925
0c1a614
98aeab4
01e21ef
8175e17
39ec2be
5e9110f
bebb375
07f777d
e565f22
60598fa
14fda90
c854eca
23e2c41
3ba090b
7e14f5b
f5866ab
2fb0914
ebda83a
5e77c2f
31e82e1
c1f3ff1
a5747a4
a19f428
ade4d2a
12719db
04d3681
b402e4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,20 @@ func pathFetchCA(b *backend) *framework.Path { | |
} | ||
} | ||
|
||
// Returns the CA chain in raw format | ||
func pathFetchCAChain(b *backend) *framework.Path { | ||
return &framework.Path{ | ||
Pattern: `ca/chain`, | ||
|
||
Callbacks: map[logical.Operation]framework.OperationFunc{ | ||
logical.ReadOperation: b.pathFetchReadCAChain, | ||
}, | ||
|
||
HelpSynopsis: pathFetchHelpSyn, | ||
HelpDescription: pathFetchHelpDesc, | ||
} | ||
} | ||
|
||
// Returns the CRL in raw format | ||
func pathFetchCRL(b *backend) *framework.Path { | ||
return &framework.Path{ | ||
|
@@ -96,6 +110,37 @@ func (b *backend) pathFetchCertList(req *logical.Request, data *framework.FieldD | |
return logical.ListResponse(entries), nil | ||
} | ||
|
||
func (b *backend) pathFetchReadCAChain(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { | ||
caInfo, err := fetchCAInfo(req) | ||
switch err.(type) { | ||
case errutil.UserError: | ||
return nil, | ||
errutil.UserError{Err: fmt.Sprintf("Could not fetch the CA certificate: %s", err)} | ||
case errutil.InternalError: | ||
return nil, | ||
errutil.InternalError{Err: fmt.Sprintf("Error fetching CA certificate: %s", err)} | ||
} | ||
|
||
caChain := caInfo.GetCAChain() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this return nil if there is no certs? Then you can easily fast-path that with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may make sense. This is only used in two places right now and should be pretty easy to transition to this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning nil here bypasses the Response where we want to provide raw output (this is currently a raw endpoint). When I returned nil here, it fast-path to a default response of 404 with an empty error array. I think we should just keep it the way it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you're right -- a GET will return a 404 on no data returned, unlike a POST or DELETE. Keeping the way it is sounds fine but if there's no data we should return 204, not 200. |
||
certs := []byte{} | ||
for _, ca := range caChain { | ||
block := pem.Block{ | ||
Type: "CERTIFICATE", | ||
Bytes: ca.Bytes, | ||
} | ||
certs = append(certs, pem.EncodeToMemory(&block)...) | ||
} | ||
|
||
response := &logical.Response{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tweaks me a little in a consistency sense.
It feels like The reason for having both is so that the Vault CLI (and API clients, generally) can do a read against one of the endpoints and understand how to parse it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both You do bring up a valid point about API clients, which I didn't fully consider. I'll think about the best way to support the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was able to merge the fetching of the ca chain into the standard fetch method. It is a little different in how this is generated vs. the rest of the certs but I think it is ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neet-o! |
||
Data: map[string]interface{}{ | ||
logical.HTTPContentType: "application/pkix-cert", | ||
logical.HTTPRawBody: certs, | ||
}} | ||
response.Data[logical.HTTPStatusCode] = 200 | ||
|
||
return response, nil | ||
} | ||
|
||
func (b *backend) pathFetchRead(req *logical.Request, data *framework.FieldData) (response *logical.Response, retErr error) { | ||
var serial, pemType, contentType string | ||
var certEntry, revokedEntry *logical.StorageEntry | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,15 +140,6 @@ func (b *backend) pathSetSignedIntermediate( | |
} | ||
} | ||
|
||
// If only one certificate is provided and it's a CA | ||
// the parsing will assign it to the IssuingCA, so move it over | ||
if inputBundle.Certificate == nil && inputBundle.IssuingCA != nil { | ||
inputBundle.Certificate = inputBundle.IssuingCA | ||
inputBundle.IssuingCA = nil | ||
inputBundle.CertificateBytes = inputBundle.IssuingCABytes | ||
inputBundle.IssuingCABytes = nil | ||
} | ||
|
||
if inputBundle.Certificate == nil { | ||
return logical.ErrorResponse("supplied certificate could not be successfully parsed"), nil | ||
} | ||
|
@@ -179,15 +170,6 @@ func (b *backend) pathSetSignedIntermediate( | |
return nil, fmt.Errorf("saved key could not be parsed successfully") | ||
} | ||
|
||
equal, err := certutil.ComparePublicKeys(parsedCB.PrivateKey.Public(), inputBundle.Certificate.PublicKey) | ||
if err != nil { | ||
return logical.ErrorResponse(fmt.Sprintf( | ||
"error matching public keys: %v", err)), nil | ||
} | ||
if !equal { | ||
return logical.ErrorResponse("key in certificate does not match stored key"), nil | ||
} | ||
|
||
inputBundle.PrivateKey = parsedCB.PrivateKey | ||
inputBundle.PrivateKeyType = parsedCB.PrivateKeyType | ||
inputBundle.PrivateKeyBytes = parsedCB.PrivateKeyBytes | ||
|
@@ -196,6 +178,10 @@ func (b *backend) pathSetSignedIntermediate( | |
return logical.ErrorResponse("the given certificate is not marked for CA use and cannot be used with this backend"), nil | ||
} | ||
|
||
if err := inputBundle.Verify(); err != nil { | ||
return nil, fmt.Errorf("verification of parsed bundle failed: %s", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for removing this? This is ensuring that the certificate that you're setting matches the backend's stored key. Seems like a decent check to have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see; this is handled later in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. All the chain verification logic was consolidated in |
||
} | ||
|
||
cb, err = inputBundle.ToCertBundle() | ||
if err != nil { | ||
return nil, fmt.Errorf("error converting raw values into cert bundle: %s", err) | ||
|
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.
The text for this probably needs some changes to accommodate the new info.
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.
I think I need to add the ca_chain to a few other documents too.