Change storage of PKI entries from colons to hyphens #2575
Conversation
lookup/migration path Still TODO: tests on migration path Fixes #2552
jefferai
force-pushed
the
pki-colons-to-hyphens
branch
from
47035cf to
dba2de5
Compare
|
I figured out why it was failing. I was passing in the JSON encoded cert DER, but for valid certs I was still using PEM format. I wonder why I was passing half of the time before though... |
| Schema: pathSetSignedIntermediate(b).Fields, | ||
| } | ||
|
|
||
| resp, err := b.pathSetSignedIntermediate(req, fd) |
There was a problem hiding this comment.
On a second thought, I think I should be using b.HandleRequest(req) here instead, with the appropriate req.Path and req.Data. Is that correct @jefferai?
There was a problem hiding this comment.
Yep, that's the best approach usually. Also it takes care of setting the field data for you!
| }, | ||
| } | ||
|
|
||
| resp, err := b.pathIssueSignCert(req, fd, role, false, false) |
There was a problem hiding this comment.
Same here, use b.HandleRequest(req)?
There was a problem hiding this comment.
I've changed the other tests to use b.HandleRequest(req) where appropriate, but not sure what to do with this one, since it's not really an exposed path.
| Schema: pathGenerateRoot(b).Fields, | ||
| } | ||
|
|
||
| resp, err := b.pathCAGenerateRoot(req, fd) |
There was a problem hiding this comment.
Same here, use b.HandleRequest(req)?
| Schema: pathSignIntermediate(b).Fields, | ||
| } | ||
|
|
||
| resp, err := b.pathCASignIntermediate(req, fd) |
|
@jefferai I took a look at code around references for |
jefferai
changed the title
|
I think this LGTM, adding Chris for another look. |
jefferai
changed the title
builtin/logical/pki/cert_util.go
Outdated
| // If we get here we need to check for old-style paths using colons | ||
| switch { | ||
| case strings.HasPrefix(prefix, "revoked/"): | ||
| path = "revoked/" + strings.Replace(strings.ToLower(serial), "-", ":", -1) |
There was a problem hiding this comment.
Instead of flipping the path back and forth, could we store two path variables and use them as we need them? I think this would improve the readability of this code.
| "certs/", | ||
| "00:00:00:00:00:00:00:00", | ||
| }, | ||
| "revoked cert": { |
There was a problem hiding this comment.
Could we add some cases that do not need the upgrade?
There was a problem hiding this comment.
https://github.com/hashicorp/vault/pull/2575/files/74965a87af47099b859552fdf2674038228a2c2e#diff-d9cca2f9a1b12f082084eaa700fff503R72 handles the cases for valid/revoked certs where the underlying path is already hyphenated so that there is no need to update the paths.
|
I can't approve my own PR. :-/ |
builtin/logical/pki/cert_util.go
Outdated
| return nil, nil | ||
| } | ||
|
|
||
| // Save the desired path | ||
| desiredPath := path |
There was a problem hiding this comment.
This path is no longer needed. You can use hyphenSerial.
There was a problem hiding this comment.
Oh nevermind. That does not work. That was also one of the reason I wanted to move away from the multiple replacements... Maybe it makes sense to create the legacyPath and path instead of just tracking the serial.
There was a problem hiding this comment.
Actually, path in this case includes the prefix, so I think we have to save it as desiredPath instead of simply using hyphenSerial.
| @@ -196,7 +197,7 @@ func (b *backend) pathSetSignedIntermediate( | |||
| return nil, err | |||
| } | |||
|
|
|||
| entry.Key = "certs/" + cb.SerialNumber | |||
| entry.Key = "certs/" + strings.ToLower(strings.Replace(cb.SerialNumber, ":", "-", -1)) | |||
There was a problem hiding this comment.
Can we have this creation of hyphenSerial in a function and use it in all the places? Seems like we are doing it in a couple of places. Easy to miss out if some if we make changes to this.
builtin/logical/pki/cert_util.go
Outdated
| @@ -216,22 +218,12 @@ func fetchCertBySerial(req *logical.Request, prefix, serial string) (*logical.St | |||
| } | |||
|
|
|||
| // No point checking these, no old/new style colons/hyphens | |||
There was a problem hiding this comment.
I don't think this comment makes sense any more. We should keep the check with path for clarity, or update the comment to say why return if legacyPath is empty.
Also add a lookup/migration path
Fixes #2552