feat: add features regarding getMetadata and signing #3

OliverShang · 2023-05-22T03:44:14Z

Features including getMetadata, describeKey and signing.

.idea/.gitignore

cmd/notation-hc-vault/main.go

cmd/notation-hc-vault/metadata.go

internal/keyvault/keyvault.go

cipherboy

I think @Two-Hearts has provided some more detailed feedback, so I'm focusing more on higher-level project feedback. :-)

Overall, from a Vault integration perspective, I think this mostly looks good!

As far as testing goes, @ncabatoff has been working on a a new Docker-based external testing interface. github.com/hashicorp/vault/sdk is meant to be imported by other projects and so would be a great fit for spinning up a Vault instance for testing. I think this example in PKI's test suite might give you an idea for using it. :-)

Let me know if you have any questions, excited for this!

internal/keyvault/keyvault.go

cipherboy · 2023-05-22T17:05:19Z

internal/keyvault/keyvault.go

+	}
+
+	signature := resp.Data["signature"].(string)
+	items := strings.Split(signature, ":")


Might be good to verify that there are three elements, that the first is vault and the second begins with a v and is a (version) number.

I'm not sure if there's a place we could save the version somewhere (in case we wish to verify against exactly that key version in the future), but if not, we can probably discard it for now :-)

cipherboy · 2023-05-22T19:08:39Z

internal/keyvault/keyvault.go

+
+func (vw *VaultClientWrapper) GetCertificateChain(ctx context.Context) ([]*x509.Certificate, error) {
+	// read a certChain
+	secret, err := vw.vaultClient.Secrets.KVv2Read(ctx, vw.keyID)


This is the part I'd ideally like to replace with a pure-Transit solution. I think hopefully with a community member's help on the Vault side, this could be done purely via Transit (one secrets engine/mount point) and not via two (KVv2 for x509 cert storage and Transit otherwise).

But for now, this part looks good.

Is there a desire to target all versions of Vault, or could we perhaps institute a higher minimum version in the future (like 1.15, which isn't out and will hopefully be the version containing native x509 support in Transit)?

@cipherboy It would be great if Transit could support uploading/downloading certificate chain in the future. For now, looks like we need to stick with the KVv2 option.

cipherboy · 2023-05-22T19:12:09Z

internal/keyvault/keyvault.go

+		log.Fatal("Error loading vault address")
+	}
+
+	VAULTTOKEN = os.Getenv("VAULT_TOKEN")


I mentioned this over on the proposal:

But one of the issues with this approach is that the environment variables will need to be modified whenever the Token expires and needs to be re-fetched. While you can use the root token, it is a highly privileged token and ideally you'd like to scope this service down to just the permissions it needs to run (ideally, just reading the certificate and signing the bundle).

I suggested on the thread that using Vault Agent could work well here: rather than explicitly configuring a Vault Token (or equivalently, handling authentication to Vault using something like AppRole/cert auth/...), you talk to Agent which acts like a transparent proxy and injects authentication into your request for you.

Agent handles the authentication & lifecycle of the token, you just talk (ideally over localhost) to Vault Agent (without authentication) and it otherwise "behaves" like a Vault server in that it injects the token into your request before passing it to the Vault server and then back to you.

Its one more dependency for those running this service, but very attractive since it handles a bit of complexity for you.

(Concretely, what this would mean would be making VAULT_TOKEN optional.)

Anyhow, give it a thought :-)

Thanks @cipherboy, yeah, I agree. The Vault Agent is definitely something we need to have in a production environment.

internal/keyvault/keyvault.go

FeynmanZhou · 2023-05-30T02:37:10Z

@OliverShang Could you add DCO by using git commit -s -m "xxx" to sign-off your commit?

Signed-off-by: olivershang <[email protected]>

This reverts commit 871f370. Signed-off-by: olivershang <[email protected]>

Signed-off-by: olivershang <[email protected]>

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]> Signed-off-by: olivershang <[email protected]>

OliverShang · 2023-05-30T08:27:04Z

@OliverShang Could you add DCO by using git commit -s -m "xxx" to sign-off your commit?

@FeynmanZhou Sure, I have fixed this issue.

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Signed-off-by: olivershang <[email protected]>

cipherboy · 2023-05-30T13:15:01Z

internal/keyvault/keyvault.go

+
+func (vw *VaultClientWrapper) SignWithTransit(ctx context.Context, encodedData string, signAlgorithm string) ([]byte, error) {
+	// sign with transit SE
+	resp, err := vw.vaultClient.Secrets.TransitSign(ctx, vw.keyID, schema.TransitSignRequest{


One additional commentary here that I think is worth discussing: this approach hides the mount path problem currently.

See: vault.WithMountPath("my/approle/path") in the README: https://github.com/hashicorp/vault-client-go

Currently this requires kv and transit plugins to be mounted at the default paths. We probably want the user to be able to specify a custom path (including namespaces)... Maybe via environment variable or config file?

Oh that makes sense. I think using a config file is a good idea.

Signed-off-by: olivershang <[email protected]>

Two-Hearts

LGTM as a first version of the project.

cipherboy

Vault side of things looks good from my PoV :-)

shizhMSFT

LGTM

Two-Hearts requested changes May 24, 2023

View reviewed changes

cipherboy reviewed May 26, 2023

View reviewed changes

OliverShang requested a review from shizhMSFT as a code owner May 30, 2023 07:25

OliverShang and others added 8 commits May 30, 2023 16:25

Added features regarding getMetadata and signing

fd4439d

Signed-off-by: olivershang <[email protected]>

Update version.go

9ced7b9

Signed-off-by: olivershang <[email protected]>

Revert "Update version.go"

88d4c9e

This reverts commit 871f370. Signed-off-by: olivershang <[email protected]>

Update version.go

993c9af

Signed-off-by: olivershang <[email protected]>

Updated gitignore and deleted .idea folder

cacea9a

Signed-off-by: olivershang <[email protected]>

Updated version.go

4c1d216

Signed-off-by: olivershang <[email protected]>

Update cmd/notation-hc-vault/main.go, implemented the help function

19018d3

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]> Signed-off-by: olivershang <[email protected]>

Update the project URL in cmd/notation-hc-vault/metadata.go

d30ebc0

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]> Signed-off-by: olivershang <[email protected]>

OliverShang force-pushed the feature/getMetadate_sign branch from 0dca167 to d30ebc0 Compare May 30, 2023 08:25

OliverShang and others added 10 commits May 30, 2023 16:28

Update internal/keyvault/keyvault.go

36333c0

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Update internal/keyvault/keyvault.go

fd5ac14

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Update internal/keyvault/keyvault.go

bfc97d8

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Update internal/keyvault/keyvault.go

9e2dffb

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Update internal/keyvault/keyvault.go

facc442

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Update internal/keyvault/keyvault.go

68321f4

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Update internal/keyvault/keyvault.go

90bfb9a

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Update internal/keyvault/keyvault.go

36096d9

Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Bingqi Shang <[email protected]>

Removed log.fatal

3836d12

Signed-off-by: olivershang <[email protected]>

Removed key version parameter as it is 0 by default

7228e73

Signed-off-by: olivershang <[email protected]>

cipherboy reviewed May 30, 2023

View reviewed changes

cipherboy mentioned this pull request Jun 2, 2023

Transit Secret Backend: x509 support? hashicorp/vault#3845

Closed

OliverShang added 3 commits June 10, 2023 14:45

replaced vault-client-go by vault api

8b5b0b4

Signed-off-by: olivershang <[email protected]>

removed comment lines

9f86df1

Signed-off-by: olivershang <[email protected]>

removed set token lines since it would be set automatically

bcd565a

Signed-off-by: olivershang <[email protected]>

Two-Hearts approved these changes Jun 20, 2023

View reviewed changes

cipherboy approved these changes Jun 20, 2023

View reviewed changes

shizhMSFT changed the title ~~feature: Add features regarding getMetadata and signing~~ feat: add features regarding getMetadata and signing Jun 26, 2023

shizhMSFT approved these changes Jun 26, 2023

View reviewed changes

shizhMSFT merged commit cd61e2c into notaryproject:main Jun 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add features regarding getMetadata and signing #3

feat: add features regarding getMetadata and signing #3

OliverShang commented May 22, 2023

cipherboy left a comment

cipherboy May 22, 2023 •

edited

Loading

cipherboy May 22, 2023

Two-Hearts May 29, 2023

cipherboy May 22, 2023

Two-Hearts May 29, 2023

FeynmanZhou commented May 30, 2023

OliverShang commented May 30, 2023 •

edited

Loading

cipherboy May 30, 2023

OliverShang Jun 11, 2023

Two-Hearts left a comment

cipherboy left a comment

shizhMSFT left a comment

feat: add features regarding getMetadata and signing #3

feat: add features regarding getMetadata and signing #3

Conversation

OliverShang commented May 22, 2023

cipherboy left a comment

Choose a reason for hiding this comment

cipherboy May 22, 2023 • edited Loading

Choose a reason for hiding this comment

cipherboy May 22, 2023

Choose a reason for hiding this comment

Two-Hearts May 29, 2023

Choose a reason for hiding this comment

cipherboy May 22, 2023

Choose a reason for hiding this comment

Two-Hearts May 29, 2023

Choose a reason for hiding this comment

FeynmanZhou commented May 30, 2023

OliverShang commented May 30, 2023 • edited Loading

cipherboy May 30, 2023

Choose a reason for hiding this comment

OliverShang Jun 11, 2023

Choose a reason for hiding this comment

Two-Hearts left a comment

Choose a reason for hiding this comment

cipherboy left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

cipherboy May 22, 2023 •

edited

Loading

OliverShang commented May 30, 2023 •

edited

Loading