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.

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 support for Secret Store client keys #392

Merged
merged 4 commits into from Feb 7, 2023

Conversation

joeshaw
Copy link
Member

@joeshaw joeshaw commented Feb 3, 2023

This lets clients create a client key for local encryption of secrets
before uploading to the Fastly API.

This bumps the minimum required Go version to 1.17 1.18 and introduces
golang.org/x/crypto as a dependency.

@joeshaw
Copy link
Member Author

joeshaw commented Feb 3, 2023

@Integralist @awilliams-fastly I don't have write access to this repo so I pushed to a fork. I also can't set reviewers so I'm tagging you in!

@joeshaw
Copy link
Member Author

joeshaw commented Feb 3, 2023

Bleh, the linter wants Go 1.19. I'm not entirely sure why the version on honnef.co/go/tools got bumped. I suspect it has to do with bringing in the crypto package.

Let me know what you want me to do. I can either try to find an older version of the crypto package that works with the older tools package, or we can bump up and require Go 1.19.

Even if we go with the older version of the crypto package, we will need to bump the requirement to Go 1.17 because the crypto API requires casting a slice to a pointer of a fixed-size array.

@Integralist
Copy link
Collaborator

👋🏻 @joeshaw you've been given WRITE access to this repo now.

It's been suggested that the API client should be using two versions back from the latest release, so that would suggest 1.18 is fine to bump to here if you want to (or if that helps with finding an older crypto package).

fastly/secret_store.go Outdated Show resolved Hide resolved
fastly/secret_store_test.go Outdated Show resolved Hide resolved
@joeshaw joeshaw force-pushed the joeshaw/secret-store-client-keys branch from 8b1dc59 to d50d58c Compare February 6, 2023 17:20
This lets clients create a client key for local encryption of secrets
before uploading to the Fastly API.

This bumps the minimum required Go version to 1.18 and introduces
`golang.org/x/crypto` as a dependency.
@joeshaw joeshaw force-pushed the joeshaw/secret-store-client-keys branch from d50d58c to e2b51cf Compare February 6, 2023 17:22
@joeshaw
Copy link
Member Author

joeshaw commented Feb 6, 2023

Thanks for the review! I've rebased on main, fixed up the module versioning, and made Go 1.18 the minimum required version.

The CI seems to be failing with the make check-mod target, but I can't reproduce the failure locally. make check-mod and go mod tidy are clean for me (trying 1.18.10 and 1.19.5)

@joeshaw
Copy link
Member Author

joeshaw commented Feb 6, 2023

Ok, I'm able to reproduce the CI failure if I clear out my module cache with go clean -modcache. I think the difference in behavior is due to this (from go help mod download):

With no arguments, download applies to the modules needed to build and test
the packages in the main module: the modules explicitly required by the main
module if it is at 'go 1.17' or higher, or all transitively-required modules
if at 'go 1.16' or lower.

(Previously the go.mod file was go 1.16.)

From the `go help mod download` docs:

> With no arguments, download applies to the modules needed to build and
test the packages in the main module: the modules explicitly required by
the main module if it is at 'go 1.17' or higher, or all
transitively-required modules if at 'go 1.16' or lower.

This causes `go mod tidy` to download additional modules not fetched by
`go mod download`.  The old check errored if there was any output from
`go mod tidy -v`.  So now we omit "downloading" messages.  As a result,
it's also no longer necessary to separately run `go mod download`, as
any modules that are needed are downloaded by `go mod tidy`.
@joeshaw
Copy link
Member Author

joeshaw commented Feb 6, 2023

Ok, latest commit fixes the CI error. go mod tidy -v downloading modules is fine. It's only when a module should be added or removed (not downloaded) that it should error out. So we now filter out downloading messages from the output, and we no longer need to go mod download as a separate step.

@joeshaw joeshaw force-pushed the joeshaw/secret-store-client-keys branch from 2a436d5 to 3861879 Compare February 6, 2023 20:52
@joeshaw
Copy link
Member Author

joeshaw commented Feb 6, 2023

Finally CI passes! 😅 staticcheck had to be upgraded to 2022.1.3 support generics (Go 1.18) but couldn't be updated to the latest release (2023.1) because it requires Go 1.19.

fastly/secret_store.go Outdated Show resolved Hide resolved
@joeshaw joeshaw force-pushed the joeshaw/secret-store-client-keys branch from f1914ff to 5a09a2f Compare February 7, 2023 18:28
// https://pkg.go.dev/golang.org/x/crypto/nacl/box#SealAnonymous
// https://libsodium.gitbook.io/doc/public-key_cryptography/sealed_boxes
func (ck *ClientKey) Encrypt(plaintext []byte) ([]byte, error) {
if len(ck.PublicKey) != 32 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use ed25519.PublicKeySize instead of the 'magic number'?

Suggested change
if len(ck.PublicKey) != 32 {
if len(ck.PublicKey) != ed25519.PublicKeySize {

Copy link
Member Author

@joeshaw joeshaw Feb 7, 2023

Choose a reason for hiding this comment

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

No, because these are actually not the same thing conceptually. The signing key is an Ed25519 public key, and uses the APIs from https://pkg.go.dev/crypto/ed25519. The client key is an X25519 public key, and uses the APIs from https://pkg.go.dev/golang.org/x/crypto/nacl/box. They're derived from similar things but aren't quite the same. This Stack Overflow answer goes into some details on the differences.

Both are 32 bytes long so it would work, but it doesn't feel quite right to use the ed25519 values here. Unfortunately, the nacl/box package hardcodes 32-byte arrays throughout its API and doesn't define a constant for this.

@Integralist Integralist merged commit 51b1099 into fastly:main Feb 7, 2023
@Integralist Integralist added the enhancement New feature or request label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants