-
Notifications
You must be signed in to change notification settings - Fork 2
Implement Crypto Host Capability #189
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
base: main
Are you sure you want to change the base?
Conversation
b3aa638
to
93c28a7
Compare
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
Signed-off-by: Esosa Ohangbon <esosaohangbon68@gmail.com>
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.
@esosaoh check why the tests are failing.
*/ | ||
export interface Certificate { | ||
encoding: CertificateEncoding; | ||
data: number[]; |
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.
data
must always be utf8, we should document it on the type here.
We could provide helper functions, but please skip if it feels too much and not language native:
export namespace CertificateUtils {
export function fromString(certString: string, encoding: CertificateEncoding): Certificate {
return {
encoding,
data: Array.from(new TextEncoder().encode(certString)), // always UTF-8
};
}
export function toString(cert: Certificate): string {
return new TextDecoder('utf-8').decode(Uint8Array.from(cert.data));
}
}
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.
Moreover, I think that number
is not a good way to represent a list of u8
. I think we should be using Uint8Array
instead
* a chain to validate it with | ||
*/ | ||
export interface CertificateVerificationRequest { | ||
cert: Certificate; |
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 should document here that:
cert
is the certificate to verify.
cert_chain
is a list of certs, ordered by trust usage (intermediates first, root last). If empty or missing, certificate is assumed trusted.
const requestObj: CertificateVerificationRequest = { | ||
cert, | ||
cert_chain: certChain, | ||
not_after: notAfter || '', |
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'm afraid the empty not_after
is enough. In the case it is not provided, we need to construct a CertificateVerificationRequest
without the not_after
key instead, as the spec says "undefined", not "empty".
This is how it is implemented in other SDKs, for example on the Rust one, the field doesn't get serialized if it's a None:
https://github.com/kubewarden/policy-sdk-rust/blob/main/src/host_capabilities/mod.rs#L131
*/ | ||
export interface CertificateVerificationRequest { | ||
cert: Certificate; | ||
cert_chain: Certificate[]; |
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.
Both cert_chain
and not_after
are optional, hence they may be missing from the JSON that a policy sends to the host.
It seems that the cert_chain
may be a None or an empty list, but for the not_after
we need to not provide it to skip it.
* CertificateVerificationRequest holds information about a certificate and | ||
* a chain to validate it with | ||
*/ | ||
export interface CertificateVerificationRequest { |
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 don't need to export this, it's a helper internal type.
This also allows us to build the Request as needed, given the optional cert_chain
and not_after
that can be missing.
This PR is blocked by kubewarden/kwctl#1337 |
*/ | ||
export interface Certificate { | ||
encoding: CertificateEncoding; | ||
data: number[]; |
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.
Moreover, I think that number
is not a good way to represent a list of u8
. I think we should be using Uint8Array
instead
Description
Fix #179
This PR implements the
v1/is_certificate_trusted
host capability as specified in hereTest
To test this pull request, you can run the following commands: