Conversation
5e32cfd to
28c3d96
Compare
ccdf027 to
6d85f02
Compare
This commit adds the GetBigInt method to the URI. This method can be useful to decode serial numbers.
I think that's OK. In general, when a specific backing KMS doesn't support a certain functionality, and still instructed to do so, it should fail. Technically, the |
| type KeyDeleter interface { | ||
| DeleteKey(req *DeleteKeyRequest) error | ||
| } | ||
|
|
There was a problem hiding this comment.
When considering/introducing these new interfaces, we may want to add a ctx as the first argument to make them more future-proof.
The main reason is I foresee us refactoring existing KMS implementations and the requests to take one too, so that we can cancel requests to remote KMSs and carry OpenTelemetry data, for example.
If we really want to keep the old function signatures we could pass a ctx in the requests, but that's not idiomatic Go. It might be an option to do that temporarily, so that not everything breaks, but then we'd have to work on the same parts at least twice.
There was a problem hiding this comment.
We are already adding an interface like this in other places, here I'm making it public. If we want to add the context, I think we could add It everywhere, using one of this options:
- ctx argument in functions, perhaps using
apiv2 - Context field in requests
- Something like
http.NewRequestWithContextthat sets an private field, that can be accessed by aContext()method, that's more or less like2the only differences is that in that method we can returncontext.Background()if it's not set instead of adding if conditions in all the methods where contexts are used.
There was a problem hiding this comment.
I would go with option 1. I'd say it would be OK to keep apiv1, as the request types themselves don't change or get a different meaning, or something like that.
kms/platform/kms_windows.go
Outdated
| // When storing certificate skip key validation. | ||
| // This avoid a prompt looking for an SmartCard. | ||
| uv.Set("skip-find-certificate-key", "true") |
There was a problem hiding this comment.
Should this happen for all calls? It is now done unconditionally.
There was a problem hiding this comment.
Only for those that store certificates, but right now I don't have different methods or an argument indicating which kind of transformation we want. This could be an improvement but I think this is the only use case for it.
|
@hslatman I've fixed most of your comments except the ones related to
|
This commit adds support for custom attestation using the attestation client and adds CreateAttestation tests.
Options 1 and 3 both don't result in Option 2 sounds good to me. The creation of an AK with |
It looks OK. Given the current interfaces and existing implementation, I think it can be added like that. The way the |
This commit adds the helper uri.Values which returns url.Values merging the ones in the opaque and query strings.
|
@hslatman I've pushed the changes for hw. Now it only sets As it is I think the only thing missing would be to error on softens if hw is set, I will need to refactor all the |
This is a first attempt to create a platform KMS. This KMS is supports mackms, tpmkms, softkms, and capikms.
There's a couple of things that I'll like to consider:
hwthe right argument, or should we useak?hw(orak) is set for softkms and capikms?