-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Vault 11795 vault cli verify s ign #18437
Conversation
Co-authored-by: 'Alex Scheel' <[email protected]>
Looking like a good start! |
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'd like Steve to take a look still, but I think this addresses my earlier review feedback. Looking good!
return 0 | ||
} | ||
|
||
func verifySignBetween(client *api.Client, issuerPath string, issuedPath string) (error, map[string]bool) { |
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.
A nit for a future PR: Go return arg order is typically reversed, with the error
/ok boolean being the last item.
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.
Looks great, a few nits but nothing preventing merging.
return fmt.Errorf("error: unable to fetch issuer %v: %w", issuerPath, err), nil | ||
} | ||
if len(issuedPath) <= 2 { | ||
return fmt.Errorf(fmt.Sprintf("%v", issuedPath)), nil |
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.
nit: no need for fmt.Sprintf
within a fmt.Errorf
|
||
pathMatch := false | ||
for _, cert := range caChain { | ||
if strings.TrimSpace(cert) == strings.TrimSpace(issuerCertPem) { // TODO: Decode into ASN1 to Check |
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.
nit: Was this a future TODO or does this need to be addressed sooner? Similar for the TODO below on line 177
New PKI CLI command that allows someone to check the relationship between a potential issuer, and a certificate it potentially issued.
[x] Tests Added - more ideas welcome
[x] Help (man?) pages exist
Functionality Test:
Docs to come in a separate PR.