Initial support for OCI key manager #4
Conversation
| func (om *OCIManager) DeleteKey(keyAttributes *model.KeyAttributes) error { | ||
| err := om.client.DeleteKey(keyAttributes.OciSecretId) | ||
| if err != nil { | ||
| log.Errorf("Error while deleting key: %s", err.Error()) |
There was a problem hiding this comment.
use errors.wrap() for consistency.
There was a problem hiding this comment.
I'd suggest to re-consider the usage of github.com/pkg/errors since it's archived and not maintained. In general, projects are trying to move away from it and use error wrapping from the std library.
There was a problem hiding this comment.
I'll update it to errors.Wrap() for now. I was just copying the code from vault_key_manager.go:DeleteKey(). If we want to move the std library implementation, I would suggest doing that as a separate patch(set) that updates error usage everywhere.
I'm also not 100% sure what wrapping from the std library you want to do -- I don't see an errors.Wrap() function there. Do you mean using fmt.Errorf() or errors.Join()?
| req := vault.CreateSecretRequest{ | ||
| CreateSecretDetails: vault.CreateSecretDetails{ | ||
| // Required | ||
| CompartmentId: common.String(keyAttributes.OciCompartmentId), |
There was a problem hiding this comment.
just a thought, would it be better to pass only the required arguments to this function instead of keyAttributes struct, just to keep the client independent of keyAttributes implementation.
There was a problem hiding this comment.
I went back and forth on that one. In the end, I again decided to model it after the Vault implementation. vaultclient.go:CreateKey() also takes a KeyAttributes struct.
I'm happy to change it to take the required arguments, though. I can also (in a separate pull request) also similarly update the vaultclient code.
There was a problem hiding this comment.
checked vaultclient implementation, its bit different in the sense that it sends entire KeyAttributes struct to backend. I would let @hmgowda comment on updating vaultclient code.
| return nil, err | ||
| } | ||
|
|
||
| return []byte(decodedStr), nil |
There was a problem hiding this comment.
DecodeString returns []byte so no need to typecast.
| // Send the request using the secrets client. | ||
| resp, err := oc.sc.GetSecretBundle(context.Background(), req) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
good to wrap the error with user-friendly message.
| @@ -53,7 +53,8 @@ func (svc service) CreateKey(_ context.Context, keyCreateReq model.KeyRequest) ( | |||
|
|
|||
| var err error | |||
| var createdKey *model.KeyResponse | |||
| if keyCreateReq.KeyInfo.KeyData == "" && keyCreateReq.KeyInfo.KmipKeyID == "" { | |||
| if keyCreateReq.KeyInfo.KeyData == "" && | |||
| (keyCreateReq.KeyInfo.KmipKeyID == "" || keyCreateReq.KeyInfo.OciSecretId == "") { | |||
There was a problem hiding this comment.
assuming OciSecretId is meant for Register operation, OR should be replaced with AND.
| @@ -60,6 +60,21 @@ type KeyInfo struct { | |||
| // KMIP Key ID, if the key is already created in KMIP Backend | |||
| // example: 7110194b-a703-4657-9d7f-3e02b62f2ed8 | |||
| KmipKeyID string `json:"kmip_key_id,omitempty"` | |||
| // The OCID of the compartment where you want to create the secret. | |||
| // example: ocid1.test.oc1..<unique_ID>EXAMPLE-compartmentId-Value | |||
| OciCompartmentId string `json:"oci_compartment_id,omitempty"` | |||
There was a problem hiding this comment.
do we need to return all these fields in response? if not, then we can keep these in separate struct.
There was a problem hiding this comment.
Yeah, that's what I was thinking too. I just did this for now since the Vault and KMIP stuff was also all mixed in there. If we want to separate them out, then I can do a patch that first pulls out the Vault and KMIP stuff into separate structs, then change this patch to also be in a separate struct.
There was a problem hiding this comment.
we only have KmipKeyID field for KMIP which is needed in both request and response (none specific to vault), hence kept KmipKeyID in KeyInfo struct which is shared between request and response. If the OCI fields also need to be kept in both request and response, then we can leave them in KeyInfo struct.
There was a problem hiding this comment.
While separating out the OCI stuff, I was going over the other code. Both KeyData and KmipKeyID are actually both only used with KeyRequest as well. They are also in the KeyAttributes struct. Is the intent is that KeyAttributes and KeyInfo should roughly match?
If so, one option for the OCI code is to just put OciSecretId in KeyInfo and KeyAttributes, as that's the only one needed in both. Then I can put the rest of the OCI fields into their own struct and just include that in the KeyRequest struct
Note that (for now) these only implement stub functionality. Full functionality will be added in later commits. Update go.mod and go.sum with new dependencies for OCI SDK. Signed-off-by: Patrick Colp <[email protected]>
OCI uses different clients for different functionality. To perform operations on a key, the vaults client is used. To actually get the contents of a key, the secrets client is used. Create an instance of each client. Signed-off-by: Patrick Colp <[email protected]>
The KeyInfo struct is a union of all needed fields (for example, KMIP fields are present). Add OCI specific members to the KeyInfo struct as well. Signed-off-by: Patrick Colp <[email protected]>
The KeyAttributes struct is a union of all needed fields (for example, KMIP fields are present). Add OCI specific members to the KeyAttributes struct as well. Signed-off-by: Patrick Colp <[email protected]>
Currently just support the "BYTES" secret type (and use "BYTES_512"). The "SSH_KEY" and "PASSPHRASE" types will be added later, as will support for "BYTES_1024". Signed-off-by: Patrick Colp <[email protected]>
Signed-off-by: Patrick Colp <[email protected]>
Signed-off-by: Patrick Colp <[email protected]>
Signed-off-by: Patrick Colp <[email protected]>
Signed-off-by: Patrick Colp <[email protected]>
Signed-off-by: Patrick Colp <[email protected]>
Signed-off-by: Patrick Colp <[email protected]>
This enables the use of the OCI key manager. Signed-off-by: Patrick Colp <[email protected]>
This patchset introduces OCIKeyManager, a new key manager backed by OCI Key Vault.
Every effort was made to match the existing code structure, but I'm open to suggestions on how certain bits of code may be separated out (for example, the KeyInfo and KeyAttribute structs).