Skip to content
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

Transit backend: Create CSR's from keys in transit and import certificate chains #21081

Conversation

Gabrielopesantos
Copy link
Contributor

@Gabrielopesantos Gabrielopesantos commented Jun 8, 2023

Implementation proposal for issue #3845.

Description:
Implements the endpoints mentioned in the proposal comment. One endpoint to create CSR's from keys in transit and another one to import a certificate chain. Besides those, the export endpoint can also export certificate chains, if imported.

What's missing:

  • Documentation update
  • Test for additional feature in export endpoint
  • Certificate chain validation on import. It wasn't a "requirement" but is still worth mentioning.

x509.ParseCertificates and fixing other bugs
fix problems with sign-csr endpoint returning base64
…server error

also moved check of key types before checking if key match to endpoint handler

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…signing a csr
@Gabrielopesantos Gabrielopesantos marked this pull request as ready for review June 8, 2023 22:16
Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other comment: I'd probably update formatKeyPolicy to include the CA Chain if present. :-)

}

if keyEntry.IsPrivateKeyMissing() {
// NOTE: Shouldn't this be a client error?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure I get the question here, mind expanding?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're talking about an error in the Transit API handler before we hit the SDK, it could be, but I think we want a check here anyways as this is a public SDK.

Copy link
Contributor Author

@Gabrielopesantos Gabrielopesantos Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All possible errors, except the one for this check, on createCsr are, in my opinion, server errors so the caller is set up to return them as such, with a 500 HTTP status code. The same will happen for the error above which I don't consider a server error. I see two ways of fixing this, duplicate this check on the caller or just have a check to see if a UserError is returned and have a slightly different resp, error logic for it.

When I created this function I did not consider the SDK but now that you mention it it makes sense to keep this check here.

EDIT: Ended up doing the same as you did here.

Comment on lines +2412 to +2414
csrTemplate.Signature = nil
csrTemplate.SignatureAlgorithm = x509.UnknownSignatureAlgorithm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
csrTemplate.Signature = nil
csrTemplate.SignatureAlgorithm = x509.UnknownSignatureAlgorithm

Strictly I don't think any of these are necessary as they're defaults and this is a CSR template, not an actual CSR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to leave these overrides here just in case the template is signed? I realized this when I tried removing these lines and rerun the tests, as the template CSR used as base was signed with an RSA key and raised errors on the other key types tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is OK then. For certificates, I thought Go would override an unsupportable recommendation with the default of the key algorithm, but it looks like I was mistaken and we (Vault) actually do that work in certutil. My bad!


csrBytes, createCertReqErr = x509.CreateCertificateRequest(rand.Reader, csrTemplate, key)
case KeyType_ED25519:
// NOTE: Still check if derived is set and fail?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would for the reasons above. :-)

csrTemplate.SignatureAlgorithm = x509.UnknownSignatureAlgorithm

var csrBytes []byte
var createCertReqErr error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key could be a var key crypto.Signer and then you could make the csrBytes, ... = x509.Create... call common to all locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, must have escaped during development. Thanks for the heads up.

return false, nil
}

func (p *Policy) PersistCertificateChain(ctx context.Context, keyVersion int, certChain []*x509.Certificate, storage logical.Storage) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to call ValidateLeafCertKeyMatch from here, to ensure the chain we're getting at the SDK level is good? It should always be the leaf certificate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about this and other missing validations on SDK functions, I am in bit of a doubt what should be included on the functions and not just the API handler. For now I ended up moving a a few things that would be duplicated to the SDK functions as they are called in the API handler functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I think I'd err on putting more stuff in the SDK than in Transit's API -- this SDK package could be reused by others, and if they call the functions wrong, (with a bad chain or otherwise), we'd like to catch that and err for them, rather than forcing them to duplicate that logic in their own functions.


// Key entry certificate chain. If set, leaf certificate key matches the
// KeyEntry key
CertificateChain []*x509.Certificate `json:"certificate_chain"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'd store this as a []*x509.Certificate since I'm not sure how it'll marshal/unmarshal; I'd probably suggest using [][]byte and explicit x509.ParseCertificate(...) calls on load tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I have made this change but I'm not sure if I understood what you meant with the "how will marshal and unmarshal" work. would you mind expanding?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, forgot to expand on this before merging. I had a great demo of this last week, wish I had remember you asked this question and I would've put it here 😆

Let's take two certificates: root and leaf. If we take the root certificate and round-trip it through JSON, you'll see that the verification of the leaf fails, even though it verifies fine without the JSON.

Go code and execution

Go code example:

// You can edit this code!
// Click here and start typing.
package main

import (
	"crypto/x509"
	"encoding/json"
	"encoding/pem"
	"fmt"
)

const rootPEM = `-----BEGIN CERTIFICATE-----
MIIBnzCCAUWgAwIBAgIUYBcTTOiYMhGTyfu8NRbSzUZHmdswCgYIKoZIzj0EAwIw
HTEbMBkGA1UEAxMSTG9uZy1MaXZlZCBSb290IFgxMB4XDTIzMDgyMjEyMjYyMVoX
DTIzMDkyMzEyMjY1MVowHTEbMBkGA1UEAxMSTG9uZy1MaXZlZCBSb290IFgxMFkw
EwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE3abeYSwv7MvVf7zpD2QJ//9iuaf5bn0U
E4ZdBsp94OcFo8CwflTR0CYQywJ68wzcClvAYg4ZkYbTT+Z3xVDRS6NjMGEwDgYD
VR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFGpFtIseCP0r
RORRpB6rz08D+/ERMB8GA1UdIwQYMBaAFGpFtIseCP0rRORRpB6rz08D+/ERMAoG
CCqGSM49BAMCA0gAMEUCIQCHiXQp7fzkjZ+Ls4Ez3m8Z/zowvaKPFX03JosNxkCt
CQIgKBbbuugqm77apjHteC2tm9zHvm1BseU5BJMqOCEpv8M=
-----END CERTIFICATE-----
`
const leafPEM = `-----BEGIN CERTIFICATE-----
MIIBtzCCAV6gAwIBAgIUFfjLDOAHeo2Tns1987cYgULUysswCgYIKoZIzj0EAwIw
HTEbMBkGA1UEAxMSTG9uZy1MaXZlZCBSb290IFgxMB4XDTIzMDgyMjEyMjY1M1oX
DTIzMDgyMjEyMjcyOFowEjEQMA4GA1UEAxMHdGVzdGluZzBZMBMGByqGSM49AgEG
CCqGSM49AwEHA0IABMTjizFLVEVsCNhJbK/m565uHqsgh5wFtRZ8mJNN2L2Yg0XG
PEN/D5YiLDcLE7WTAAm2mrt1TrqdOEGOripdU+CjgYYwgYMwDgYDVR0PAQH/BAQD
AgOoMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAdBgNVHQ4EFgQUPmoJ
10pHYZMsuhRMl5Q34327d84wHwYDVR0jBBgwFoAUakW0ix4I/StE5FGkHqvPTwP7
8REwEgYDVR0RBAswCYIHdGVzdGluZzAKBggqhkjOPQQDAgNHADBEAiBrqAj9ocjR
XmzpvWmGQBxa25fNTEMWYTaCEAy8VkZRYgIgT3dyVXQGjffQyWF4o0KLNOAUH3Om
GA30tvXYKZ/8ksU=
-----END CERTIFICATE-----
`

func main() {
	rootBlock, _ := pem.Decode([]byte(rootPEM))
	rootOrig, _ := x509.ParseCertificate(rootBlock.Bytes)

	leafBlock, _ := pem.Decode([]byte(leafPEM))
	leafOrig, _ := x509.ParseCertificate(leafBlock.Bytes)

	if err := leafOrig.CheckSignatureFrom(rootOrig); err != nil {
		fmt.Printf("failed to validate original signature: %v\n", err)
	} else {
		fmt.Printf("checked original signatures: OK\n")
	}

	// XXX: this is what we were doing prior to your
	// change to [][]byte: we were storing it as a JSON-encoded
	// struct field of type *x509.Certificate -- that when stored
	// (marshalled) and read again later (unmarshalled), would
	// not be a "functioning" certificate.
	rootCertStored, _ := json.Marshal(rootOrig)
	var rootUnstored *x509.Certificate
	json.Unmarshal(rootCertStored, &rootUnstored)

	if err := leafOrig.CheckSignatureFrom(rootUnstored); err != nil {
		fmt.Printf("failed to validate round-tripped signature: %v\n", err)
		fmt.Printf("unstored root key: (type: %T) %v\n", rootUnstored.PublicKey, rootUnstored.PublicKey)
		fmt.Printf("orig root key: (type: %T) %v\n", rootOrig.PublicKey, rootOrig.PublicKey)
	} else {
		fmt.Printf("checked round-tripped signatures: OK\n")
	}
}

Execution results:

checked original signatures: OK
failed to validate round-tripped signature: x509: cannot verify signature: algorithm unimplemented
unstored root key: (type: map[string]interface {}) map[Curve:map[] X:1.0025597095711236e+77 Y:2.550890208028647e+75]
orig root key: (type: *ecdsa.PublicKey) &{0x685500 100255970957112353876818454842956629422432050733395608904658668741126008987879 2550890208028647296912217307641692388690570932323450769480190751495358042443}

https://go.dev/play/p/zX9sS8M8vk1

IOW, because (after unmarshaling the certificate) you get different types for the public key (it is an untyped interface{} after all), you can't round-trip Go's x509 certificate structure via JSON, like we do for storage here. So you're best off using a round-tripping type, so you don't have to deal with how do you "fix" the unmarshal if you want to do any more advanced operations on it in the future (e.g., verify signatures using the certificate in Vault -- for chain building/verification for instance).

builtin/logical/transit/path_export.go Outdated Show resolved Hide resolved
builtin/logical/transit/path_certificates.go Outdated Show resolved Hide resolved
Y: keyEntry.EC_Y,
}

// NOTE: Is it worth having this check?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is already handled by Go by the subsequent call, so I'm happy ignoring it AFAICT: https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/crypto/ecdsa/ecdsa.go;l=73

Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great, thank you @Gabrielopesantos!

@cipherboy cipherboy merged commit 1996377 into hashicorp:main Aug 22, 2023
socheatsok78 added a commit to socheatsok78/vault that referenced this pull request Apr 3, 2024
The `certificate_chain` parameter is incorrect from the description in the PR hashicorp#21081.
stevendpclark pushed a commit that referenced this pull request Apr 4, 2024
…#26250)

The `certificate_chain` parameter is incorrect from the description in the PR #21081.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants