Add support for additional CAPI certificate context properties #873
Conversation
|
|
hslatman
changed the title
This fixes an issue with agent, where it was unable to find device certificates due to using a key-id derived from a random string for certificate lookups. If the key-id lookup fails automatically attempt lookup by cert+attr where attr can be one of the certificate atributes, serial number, friendly-name, description, etc.
The function was a relic from past refactoring code, not necessary anymore.
joshdrake
force-pushed
the
josh/capi-set-cert-context-props
branch
from
b78b8bb to
5505db4
Compare
- Wrong variable names. - Added missing forwarding issuer,description,friendly-name params to CAPI.
| } | ||
|
|
||
| func certSetCertificateContextProperty(certContext *windows.CertContext, propID uint32, pvData uintptr) error { | ||
| r0, _, err := procCertSetCertificateContextProperty.Call( |
There was a problem hiding this comment.
What is r0? I think this could use a more descriptive name.
There was a problem hiding this comment.
It's actually r1, this invokes a system call on windows (whose procedure is found in a DLL), the system call returns r1, r2, error, in general, where r1 represents the return value status from the procedure stored in a register (e.g. on Linux the %rax value), the semantic value of this depends on the procedure invoked, r2 is usually not used but kept for compatibility with platforms that return status on more registers, and the 3rd value return is actually an error, on windows the error is always non nil so we must check r0(r1).
I think we can leave this low level stuff as is, or perhaps considering refactor this in another pr.
There was a problem hiding this comment.
Ok, that all makes sense. Can you add a comment to these funcs explaining it? With the information you provided above, it's clear, but without it, it's not clear what's happening.
| if r0 == 0 { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Is err assumed to be non-nil here?
If err != nil, and r0 is != 0, is it ok to drop the error?
There was a problem hiding this comment.
This invokes CertSetCertificateContextProperty which returns a bool, on success it returns true.
But since this is system call invocation the bool is cast to int, false is zero, so in that case we actually return the error.
We handle this as syscall on windows always return a non nil error even if the function succeeds
There was a problem hiding this comment.
Similar to the above, I think comments helps a lot here.
- Document lookup by issuer strategy. - Attempt lookup by issuer only when lookup by KeyID fails with not found, on error return. - Simplify code a little bit.
This PR adds functionality to the
capiKMS to support setting the "friendly name" and "description" certificate properties. Thetpmkmspasses these through as needed.💔Thank you!