Skip to content

feat/add static secret cryptor#151

Open
jithinkunjachan wants to merge 4 commits into
mainfrom
feat/add-static-secret-cryptor
Open

feat/add static secret cryptor#151
jithinkunjachan wants to merge 4 commits into
mainfrom
feat/add-static-secret-cryptor

Conversation

@jithinkunjachan
Copy link
Copy Markdown
Contributor

@jithinkunjachan jithinkunjachan commented Jun 4, 2026

No description provided.

@jithinkunjachan jithinkunjachan self-assigned this Jun 4, 2026
@jithinkunjachan jithinkunjachan force-pushed the feat/add-static-secret-cryptor branch 2 times, most recently from e9fd80b to c48f798 Compare June 4, 2026 11:13
@jithinkunjachan jithinkunjachan marked this pull request as ready for review June 4, 2026 11:14
Comment thread internal/cryptor/staticsecret.go Outdated
Comment thread internal/cryptor/staticsecret.go Outdated
@jithinkunjachan jithinkunjachan changed the title Feat/add static secret cryptor feat/add static secret cryptor Jun 4, 2026
Comment thread internal/cryptor/staticsecret.go Outdated
Comment thread internal/cryptor/staticsecret.go Outdated
Comment thread internal/cryptor/staticsecret.go Outdated
// Currently only [InfoNameStaticSecret] is supported. The secret must be non-nil and non-empty.
func NewStaticSecret(name InfoName, secret *securemem.Data) (*StaticSecret, error) {
if name != InfoNameStaticSecret {
return nil, ErrRequest
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can we make a specific error

Comment thread internal/cryptor/staticsecret.go Outdated
return nil, ErrRequest
}

if secret == nil || len(secret.SecureBytes()) == 0 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be len(32) right?

Comment thread internal/cryptor/staticsecret.go Outdated
}

if secret == nil || len(secret.SecureBytes()) == 0 {
return nil, ErrRequest
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can we make a specific error?

Signed-off-by: jK <33685667+jithinkunjachan@users.noreply.github.com>
Signed-off-by: jK <33685667+jithinkunjachan@users.noreply.github.com>
…and introduce CryptorSecret type

Signed-off-by: jK <33685667+jithinkunjachan@users.noreply.github.com>
@jithinkunjachan jithinkunjachan force-pushed the feat/add-static-secret-cryptor branch from a17d8f9 to ea4f363 Compare June 5, 2026 07:32
Copy link
Copy Markdown

@apatsap apatsap left a comment

Choose a reason for hiding this comment

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

Some refactoring suggestions

Comment thread internal/cryptor/aes256gcm/aes256gcm.go Outdated
@@ -34,36 +37,40 @@ var ErrAllocatedDataNotFound = errors.New("allocated data not found in vault")
// NewAES256GCM returns a ready-to-use AES-256-GCM cryptor.
func NewAES256GCM() *AES256GCM {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

aes256gcm.New


// NewStaticSecret returns a StaticSecret for the given algorithm name and key material.
// Currently only [InfoNameStaticSecret] is supported. The secret must be non-nil and non-empty.
func NewStaticSecret(name cryptor.InfoName, secret *securemem.Data) (*StaticSecret, error) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

staticsecret.New

Comment thread internal/cryptor/cryptor.go Outdated
Comment on lines +22 to +26
// InfoNameAES256GCM indicates that the Cryptor supports AES-256 in Galois/Counter Mode (GCM).
const InfoNameAES256GCM InfoName = "AES256-GCM"

// InfoNameStaticSecret indicates a Cryptor that manages its own static key material.
const InfoNameStaticSecret InfoName = "AES256-GCM-STATIC-SECRET"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

they should go into their individual pkgs

Comment thread internal/cryptor/cryptor.go Outdated
Comment on lines +122 to +129
if req.Secret != nil {
if req.Secret.Algorithm == "" {
return fmt.Errorf("invalid secret algorithm: %w", ErrRequest)
}
if req.Secret.Data == nil || len(req.Secret.Data.SecureBytes()) == 0 {
return fmt.Errorf("invalid secret data: %w", ErrRequest)
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we should put this into CryptorSecret.Validate()error and then just call it here, because we also use it in the decrypt validation.

Comment thread internal/cryptor/cryptor.go Outdated
Comment on lines +146 to +153
if req.Secret != nil {
if req.Secret.Algorithm == "" {
return fmt.Errorf("invalid secret algorithm: %w", ErrRequest)
}
if req.Secret.Data == nil || len(req.Secret.Data.SecureBytes()) == 0 {
return fmt.Errorf("invalid secret data: %w", ErrRequest)
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we should put this into CryptorSecret.Validate()error and then just call it here, because we also use it in the encrypt validation.

@jithinkunjachan jithinkunjachan requested a review from apatsap June 5, 2026 08:39
…ctors

Signed-off-by: jK <33685667+jithinkunjachan@users.noreply.github.com>
Copy link
Copy Markdown

@apatsap apatsap left a comment

Choose a reason for hiding this comment

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

Nice!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants