feat: Vault tokens are now identity-based instead of anonymous by bnevis-i · Pull Request #4327 · edgexfoundry/edgex-go

bnevis-i · 2023-02-06T22:50:26Z

This is an internal refactoring of security-secretstore-setup, security-file-token-provider, and security-spiffe-token-provider to generate Vault tokens that are based on a Vault identity instead of being anonymous tokens with an attached ACL.

The consequence of this refactoring is that the token issuing components of EdgeX run with fewer required privileges than before, and it is possible to ask Vault for a verifiable JWT-based identity assertion of an EdgeX service.

This change should be completely transparent to all existing EdgeX services.

Signed-off-by: Bryon Nevis bryon.nevis@intel.com

PR Checklist

Please check if your PR fulfills the following requirements:

I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
I am not introducing a new dependency (add notes below if you are)
I have added unit tests for the new feature or bug fix (if not, why?)
I have fully tested (add details below) this the new feature or bug fix (if not, why?)
I have opened a PR for the related docs change (if not, why?) n/a

Testing Instructions

# in edge-go
make clean_docker_base docker_base docker
# in compose-builder
make run dev ds-virtual
make get-token
# Go to localhost:4000 with the above token and make sure everything runs as usual

New Dependency Instructions (If applicable)

jim-wang-yutsung · 2023-02-06T23:03:03Z

+#PrivilegedTokenPath = "UNUSED"
+#ConfigFile = "UNUSED"
+#OutputDir = "UNUSED"
+#OutputFilename = "UNUSED"


if we remove this, shouldn't it become breaking changes in terms of update pov?

What this is actually doing is reusing the configuration.toml snippet from security-file-token-provider except that we don't need all of the parameters, just some of them (like the TTL on the vault token and JWT). I'm just making it clear that we don't use these parameters in spiffe-token-provider.

jim-wang-yutsung · 2023-02-06T23:05:04Z

 //
 // SPDX-License-Identifier: Apache-2.0'
 //
-package fileprovider
+package common


update license header for year 2023

Oh no! Just pushed a commit to update copyright dates.

jim-wang-yutsung · 2023-02-06T23:06:50Z

-		// Set a meta property that consuming serices can use to automatically scope secret queries
-		createTokenParameters["meta"] = map[string]interface{}{
-			"edgex-service-name": serviceName,
+		randomPassword, err := credentialGenerator.Generate(context.TODO())


what is context.TODO()?

TODO returns a non-nil, empty Context. Code should use context.TODO when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a Context parameter)

The original implementation of the credential generator used a key derivation scheme that derived multiple passwords from the same base secret (that code used Context). We later changed it to just do random bits in base64. Context is no longer used, but left in the API to avoid having to modify a million places in the code. Per GO BKM, never pass a nil Context.

Thanks for details explanation 👍

bnevis-i · 2023-02-07T21:35:35Z

Resolved merge conflict in go.mod.

jim-wang-yutsung · 2023-02-07T21:58:09Z

+	if identityId != "" {
+		err = m.secretStoreClient.DeleteIdentity(m.privilegedToken, username)
+		if err != nil {
+			return err
+		}
+	}
+
+	err = m.secretStoreClient.DeleteUser(m.privilegedToken, m.userPassMountPoint, username)
+	if err != nil {
+		return err
+	}


should we make this transactional or atomic? if deleting user somehow failed then you lost identity for that user isn't it?

I think its ok. The ACL is attached to the identity so if you delete the identity the user is really gone. You could attempt to log in as the user, but it would error out as it isn't bound to anything. Moreover, if you recreated the user it would overwrite the identity and/or login, so a partial deletion would take up space but that's about it.

And if you redeleted the user due to an error, the lookupidentity would not find an identity and it would proceed to delete the login.

ok, good to know that creating user would get a new identity.

This is an internal refactoring of security-secretstore-setup, security-file-token-provider, and security-spiffe-token-provider to generate Vault tokens that are based on a Vault identity instead of being anonymous tokens with an attached ACL. The consequence of this refactoring is that the token issuing components of EdgeX run with fewer required privileges than before, and it is possible to ask Vault for a verifiable JWT-based identity assertion of an EdgeX service. Signed-off-by: Bryon Nevis <bryon.nevis@intel.com>

sonarqubecloud · 2023-02-08T14:56:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jim-wang-yutsung · 2023-02-08T15:50:06Z


 	expectedService1Policy := `{"path":{"secret/non/standard/location/*":{"capabilities":["list","read"]}}}`
-	expectedService1Parameters := makeMetaServiceName("myservice")
+	//expectedService1Parameters := makeMetaServiceName("myservice")


remove the dead code

bnevis-i requested review from farshidtz, jim-wang-yutsung and lenny-goodell February 6, 2023 22:50

bnevis-i added the 1-low priority denoting isolated changes label Feb 6, 2023

bnevis-i added this to the Minnesota milestone Feb 6, 2023

bnevis-i added the security-services label Feb 6, 2023

jim-wang-yutsung reviewed Feb 6, 2023

View reviewed changes

bnevis-i requested a review from jim-wang-yutsung February 7, 2023 21:35

jim-wang-yutsung reviewed Feb 7, 2023

View reviewed changes

bnevis-i requested a review from jim-wang-yutsung February 8, 2023 15:40

jim-wang-yutsung approved these changes Feb 8, 2023

View reviewed changes

bnevis-i merged commit fd143bc into edgexfoundry:main Feb 8, 2023

Conversation

bnevis-i commented Feb 6, 2023

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnevis-i Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnevis-i Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnevis-i commented Feb 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnevis-i Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Feb 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bnevis-i Feb 6, 2023 •

edited

Loading

bnevis-i Feb 6, 2023 •

edited

Loading

bnevis-i Feb 8, 2023 •

edited

Loading