-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: Add CEL kw.crypto.verifyCert() function #18
Conversation
a547941
to
d71c662
Compare
Blocked, consuming kubewarden/policy-sdk-go#74. Once there's a new SDK, It can move forward. |
b83a011
to
03894ad
Compare
Ready for review. |
return []cel.EnvOption{ | ||
// group every binding under a container to simplify usage | ||
cel.Container("crypto"), | ||
|
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.
we are not using the request/response struct native types.
also, what about DER certificates?
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.
also, what about DER certificates?
Since Der is a binary data format that would not be fit for a string, it seemed a good idea to not provide it.
we are not using the request/response struct native types.
If we don't need a type Certificate{}
with an Encoding
field for Der, we don't need the request types.
Also, we couldn't use the Request type as-is, as the struct type capabilities/crypto/Certificate{}
expects a []rune (and by extension the struct type CertificateVerificationRequest{}
). CEL only knows about strings, bytes, and []int, and provides stringToBytes (not fit) and stringToInt64 (should be int8). We would need intermediate helper types.
For the Response type, it could be done, but at that point I preferred to return a map than a CertificateVerificationResponse{} that needs to be documented. It may also seem simpler to CEL users (but I'm open to changing it).
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.
@fabriziosestito it's my fault, I told Victor this could have been dropped because it would not be possible to write binary data into the definition of a policy.
However, @fabriziosestito reminded me CEL policies can also fetch data from external sources, like a ConfigMap. Do you think this could be a way to make DER data appear into a policy? I suspect you would still have to take the DER data, encode it using base64 and add that into a Secrets/ConfigMap. The CEL policy would have to work in reverse order: base64 decode (is there a way to do that) -> DER -> our function.
Is that something that can realistically happen? Should we just focus on the most common use case right now (PEM) and expand to support DER later on?
I'm a bit conflicted, because if we were to add DER support later on, we would have to break our CEL API; which would be super painful...
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.
We could provide 2 different CEL functions, one for Pem and one for Der. I could rename the current function.
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.
Opened #23.
I'm conflicted if to subtly change the current kw.crypto.verifyCert()
signature to make it more easier to add DER certs later on.
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
03894ad
to
a23549d
Compare
internal/cel/library/crypto.go
Outdated
|
||
return types.NewStringInterfaceMap(types.DefaultTypeAdapter, | ||
map[string]any{ | ||
"Trusted": response.Trusted, |
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: we should use lowercase keys, as it is generally what happens with the other dyn types (e.g. k8s objects used by ValidatingAdmissionPolicy)
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.
indeed, better. Done in 47740c4.
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Description
Relates to #8.
Depends on kubewarden/policy-sdk-go#74.
Fix #15.
feat: Add CEL kw.crypto.verifyCert() function.
Test
CI
Additional Information
Tradeoff
Potential improvement