Skip to content
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.

Sign up for GitHub

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

Add new interfaces to support operations on nested credentials #29

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+528 −108
Open

Add new interfaces to support operations on nested credentials #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8c444ae
add interfaces for nested credentials
gouthamMN Dec 31, 2024
382bc8f
update unit tests
gouthamMN Dec 31, 2024
f929b46
New tests for nested credentials interfaces
gouthamMN Dec 31, 2024
db4c231
separate secret response types and make secret value non ptr
gouthamMN Jan 2, 2025
f6679b7
update UT to use cmp instead reflect
gouthamMN Jan 2, 2025
777a30d
separate deleted secret response type
gouthamMN Jan 2, 2025
94370d3
update UT to verify entire response
gouthamMN Jan 2, 2025
4f436ae
update integration test
gouthamMN Jan 2, 2025
be6d299
bind to this using
gouthamMN Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ require (
github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.7.0
github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.1.0
github.com/go-playground/validator/v10 v10.22.0
github.com/google/go-cmp v0.6.0
go.uber.org/mock v0.4.0
gopkg.in/dnaeon/go-vcr.v3 v3.2.0
)
Expand All @@ -18,7 +19,6 @@ require (
github.com/go-playground/locales v0.14.1 // indirect
github.com/go-playground/universal-translator v0.18.1 // indirect
github.com/golang-jwt/jwt/v5 v5.2.1 // indirect
github.com/google/go-cmp v0.6.0 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/kylelemons/godebug v1.1.0 // indirect
github.com/leodido/go-urn v1.4.0 // indirect
Expand Down
138 changes: 113 additions & 25 deletions pkg/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
"github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets"
"github.com/Azure/msi-dataplane/pkg/dataplane"
"github.com/Azure/msi-dataplane/pkg/dataplane/swagger"
)

var (
Expand All @@ -20,11 +21,16 @@ type DeletedSecretProperties struct {
DeletedDate time.Time
}

type DeletedSecretResponse struct {
type DeletedCredentialsObjectSecretResponse struct {
CredentialsObject dataplane.CredentialsObject
Properties DeletedSecretProperties
}

type DeletedNestedCredentialsObjectSecretResponse struct {
NestedCredentialsObject swagger.NestedCredentialsObject
Properties DeletedSecretProperties
}

type MsiKeyVaultStore struct {
kvClient KeyVaultClient
}
Expand All @@ -36,18 +42,33 @@ type SecretProperties struct {
NotBefore time.Time
}

type SecretResponse struct {
type CredentialsObjectSecretResponse struct {
CredentialsObject dataplane.CredentialsObject
Properties SecretProperties
}

type NestedCredentialsObjectSecretResponse struct {
NestedCredentialsObject swagger.NestedCredentialsObject
Properties SecretProperties
}

type secretObject struct {
value string
properties SecretProperties
}

type deletedSecretObject struct {
value string
properties DeletedSecretProperties
}

func NewMsiKeyVaultStore(kvClient KeyVaultClient) *MsiKeyVaultStore {
return &MsiKeyVaultStore{kvClient: kvClient}
}

// Delete a credentials object from key vault using the specified secret name.
// Delete applies to all versions of the secret.
func (s *MsiKeyVaultStore) DeleteCredentialsObject(ctx context.Context, secretName string) error {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless this method and its siblings is adding some value for the caller - e.g. taking in the credentials object and calculating the storage path - we should omit the method entirely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

type MsiKeyVaultStore struct {
	kvClient KeyVaultClient
}

I agree with your point, but since the kvClient is private and won't be able to access outside the store package this might have to be redesigned altogether. I think original author of this was @carvalhe and tagging him here to understand the rationale behind this.

@stevekuznetsov - So, If I understand your ask properly then the possible solution is to make kvClient a public as this would let the client to directly access KV interface methods. But with this we might introduce inconsistency in the way we let the client access our methods.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of the users of this library will need to impose some keyvault key naming convention by which a specific credential or nested credential can be stored and retrieved by knowing only some identifying characteristics of the MSI. I imagine both the refresher and the clusters-service will furthermore need to agree on this pattern. We can either omit the storage piece from this library and leave it up to the end-clients or we can add value to the storage piece by amending our interfaces not to act over opaque secretNames.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can either omit the storage piece from this library and leave it up to the end-clients.

This approach needs to be discussed with RH as this will impact Classic MIWI clusters changes.

we can add value to the storage piece by amending our interfaces not to act over opaque secretNames.

Is the ask is to create a separate interface for cred obj and nested cred obj? eg:DeleteCredentialsObject() & DeleteNestedCredentialsObject()?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see how the clients use the library and decide what makes the most sense for the interface.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For MIWI classic, we're currently doing exactly what @stevekuznetsov described in an earlier comment; we impose a naming convention for the KV keys by using a utility function that takes in the cluster doc and returns the secret name, call the utility function to get the name, and then delete by name: https://github.com/Azure/ARO-RP/blob/b534984ce591b208ded342d8e1538f1fce69d837/pkg/cluster/delete.go#L372

In classic we only need to delete CredentialsObjects and not NestedCredentialsObjects though. Maybe someone from HCP would be able to provide more valuable input here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever use the result of the deletion call? I would assume not because the backing credential is actually deleted.

For HCP, CS wanted to prefix the secret name with either nco or co representing nested credential object and credential object respectively.

ARO Classic is storing the raw credentials object with the below contents, where the docID is a guid and the name is the name of the resource ID of the managed identity.

	return fmt.Sprintf("%s-%s", m.doc.ID, clusterMsi.Name), nil

Do we need to force consistent naming across the two, or can we have the refresher and ARO HCP (cluster service) leverage nco and assume everything else is a credentials object? Also is there any way to abstract the deletion to exist for both types and the response is either just a string or we don't care about it?

If we'd like consistent prefix/suffix across 3 repos (cluster service, aro classic, and msi refresher) should we move the naming prefix into this library and return the secret name back, or do we not care as a client?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we'd like consistent prefix/suffix across 3 repos (cluster service, aro classic, and msi refresher) should we move the naming prefix into this library and return the secret name back

That is my question. I assumed the answer was yes - please provide your opinions :)

func (s *MsiKeyVaultStore) DeleteSecret(ctx context.Context, secretName string) error {
if _, err := s.kvClient.DeleteSecret(ctx, secretName, nil); err != nil {
return err
}
Expand All @@ -57,7 +78,38 @@ func (s *MsiKeyVaultStore) DeleteCredentialsObject(ctx context.Context, secretNa

// Get a credentials object from the key vault using the specified secret name.
// The latest version of the secret will always be returned.
func (s *MsiKeyVaultStore) GetCredentialsObject(ctx context.Context, secretName string) (*SecretResponse, error) {
func (s *MsiKeyVaultStore) GetCredentialsObject(ctx context.Context, secretName string) (*CredentialsObjectSecretResponse, error) {
secretObject, err := s.getSecret(ctx, secretName)
if err != nil {
return nil, err
}

var credentialsObject dataplane.CredentialsObject
if err := credentialsObject.UnmarshalJSON([]byte(secretObject.value)); err != nil {
return nil, err
}

return &CredentialsObjectSecretResponse{CredentialsObject: credentialsObject, Properties: secretObject.properties}, nil
}

// Get a nested credentials object from the key vault using the specified secret name.
func (s *MsiKeyVaultStore) GetNestedCredentialsObject(ctx context.Context, secretName string) (*NestedCredentialsObjectSecretResponse, error) {
secretObject, err := s.getSecret(ctx, secretName)
if err != nil {
return nil, err
}

var nestedCredentialsObject swagger.NestedCredentialsObject
if err := nestedCredentialsObject.UnmarshalJSON([]byte(secretObject.value)); err != nil {
return nil, err
}

return &NestedCredentialsObjectSecretResponse{NestedCredentialsObject: nestedCredentialsObject, Properties: secretObject.properties}, nil
}

// Get a secret from the key vault using the specified secret name.
// The latest version of the secret will always be returned.
func (s *MsiKeyVaultStore) getSecret(ctx context.Context, secretName string) (*secretObject, error) {
// https://github.com/Azure/azure-sdk-for-go/blob/3fab729f1bd43098837ddc34931fec6c342fa3ef/sdk/security/keyvault/azsecrets/client.go#L197
latestSecretVersion := ""
secret, err := s.kvClient.GetSecret(ctx, secretName, latestSecretVersion, nil)
Expand All @@ -68,10 +120,6 @@ func (s *MsiKeyVaultStore) GetCredentialsObject(ctx context.Context, secretName
if secret.Value == nil {
return nil, errNilSecretValue
}
var credentialsObject dataplane.CredentialsObject
if err := credentialsObject.UnmarshalJSON([]byte(*secret.Value)); err != nil {
return nil, err
}

secretProperties := SecretProperties{
Name: secretName,
Expand All @@ -92,26 +140,50 @@ func (s *MsiKeyVaultStore) GetCredentialsObject(ctx context.Context, secretName
secretProperties.NotBefore = *secret.Attributes.NotBefore
}
}

return &SecretResponse{CredentialsObject: credentialsObject, Properties: secretProperties}, nil
return &secretObject{value: *secret.Value, properties: secretProperties}, nil
}

// Get a deleted credentials object from the key vault using the specified secret name.
func (s *MsiKeyVaultStore) GetDeletedCredentialsObject(ctx context.Context, secretName string) (*DeletedSecretResponse, error) {
response, err := s.kvClient.GetDeletedSecret(ctx, secretName, nil)
func (s *MsiKeyVaultStore) GetDeletedCredentialsObject(ctx context.Context, secretName string) (*DeletedCredentialsObjectSecretResponse, error) {
deletedSecretObject, err := s.getDeletedSecret(ctx, secretName)
if err != nil {
return nil, err
}

if response.Value == nil {
return nil, errNilSecretValue
var credentialsObject dataplane.CredentialsObject
if err := credentialsObject.UnmarshalJSON([]byte(deletedSecretObject.value)); err != nil {
return nil, err
}

var credentialsObject dataplane.CredentialsObject
if err := credentialsObject.UnmarshalJSON([]byte(*response.Value)); err != nil {
return &DeletedCredentialsObjectSecretResponse{CredentialsObject: credentialsObject, Properties: deletedSecretObject.properties}, nil
}

// Get a deleted nested credentials object from the key vault using the specified secret name.
func (s *MsiKeyVaultStore) GetDeletedNestedCredentialsObject(ctx context.Context, secretName string) (*DeletedNestedCredentialsObjectSecretResponse, error) {
deletedSecretObject, err := s.getDeletedSecret(ctx, secretName)
if err != nil {
return nil, err
}

var nestedCredentialsObject swagger.NestedCredentialsObject
if err := nestedCredentialsObject.UnmarshalJSON([]byte(deletedSecretObject.value)); err != nil {
return nil, err
}

return &DeletedNestedCredentialsObjectSecretResponse{NestedCredentialsObject: nestedCredentialsObject, Properties: deletedSecretObject.properties}, nil
}

// Get a deleted secret from the key vault using the specified secret name.
func (s *MsiKeyVaultStore) getDeletedSecret(ctx context.Context, secretName string) (*deletedSecretObject, error) {
response, err := s.kvClient.GetDeletedSecret(ctx, secretName, nil)
if err != nil {
return nil, err
}

if response.Value == nil {
return nil, errNilSecretValue
}

deletedSecretProperties := DeletedSecretProperties{
Name: secretName,
RecoveryLevel: "",
Expand All @@ -129,22 +201,22 @@ func (s *MsiKeyVaultStore) GetDeletedCredentialsObject(ctx context.Context, secr
}
}

return &DeletedSecretResponse{CredentialsObject: credentialsObject, Properties: deletedSecretProperties}, nil
return &deletedSecretObject{value: *response.Value, properties: deletedSecretProperties}, nil
}

// Get a pager for listing credentials objects from the key vault.
func (s *MsiKeyVaultStore) GetCredentialsObjectPager() *runtime.Pager[azsecrets.ListSecretPropertiesResponse] {
// Get a pager for listing Secret objects from the key vault.
func (s *MsiKeyVaultStore) GetSecretObjectPager() *runtime.Pager[azsecrets.ListSecretPropertiesResponse] {
return s.kvClient.NewListSecretPropertiesPager(nil)
}

// Get a pager for listing deleted credentials objects from the key vault.
func (s *MsiKeyVaultStore) GetDeletedCredentialsObjectPager() *runtime.Pager[azsecrets.ListDeletedSecretPropertiesResponse] {
// Get a pager for listing deleted Secret objects from the key vault.
func (s *MsiKeyVaultStore) GetDeletedSecretObjectPager() *runtime.Pager[azsecrets.ListDeletedSecretPropertiesResponse] {
return s.kvClient.NewListDeletedSecretPropertiesPager(nil)
}

// Purge a deleted credentials object from the key vault using the specified secret name.
// Purge a deleted Secret object from the key vault using the specified secret name.
// This operation is only applicable in vaults enabled for soft-delete.
func (s *MsiKeyVaultStore) PurgeDeletedCredentialsObject(ctx context.Context, secretName string) error {
func (s *MsiKeyVaultStore) PurgeDeletedSecretObject(ctx context.Context, secretName string) error {
if _, err := s.kvClient.PurgeDeletedSecret(ctx, secretName, nil); err != nil {
return err
}
Expand All @@ -153,16 +225,32 @@ func (s *MsiKeyVaultStore) PurgeDeletedCredentialsObject(ctx context.Context, se
}

// Set a credentials object in the key vault using the specified secret name.
// If the secret already exists, key vault will create a new version of the secret.
func (s *MsiKeyVaultStore) SetCredentialsObject(ctx context.Context, properties SecretProperties, credentialsObject dataplane.CredentialsObject) error {
credentialsObjectBuffer, err := credentialsObject.MarshalJSON()
if err != nil {
return err
}

credentialsObjectString := string(credentialsObjectBuffer)
return s.setSecret(ctx, properties, &credentialsObjectString)
}

// Set a nested credentials object in the key vault using the specified secret name.
func (s *MsiKeyVaultStore) SetNestedCredentialsObject(ctx context.Context, properties SecretProperties, nestedCredentialsObject swagger.NestedCredentialsObject) error {
nestedCredentialsObjectBuffer, err := nestedCredentialsObject.MarshalJSON()
if err != nil {
return err
}

nestedCredentialsObjectString := string(nestedCredentialsObjectBuffer)
return s.setSecret(ctx, properties, &nestedCredentialsObjectString)
}

// Set a secret in the key vault using the specified secret name.
// If the secret already exists, key vault will create a new version of the secret.
func (s *MsiKeyVaultStore) setSecret(ctx context.Context, properties SecretProperties, secretValue *string) error {
setSecretParameters := azsecrets.SetSecretParameters{
Value: &credentialsObjectString,
Value: secretValue,
SecretAttributes: &azsecrets.SecretAttributes{
Enabled: &properties.Enabled,
Expires: &properties.Expires,
Expand Down
Loading