feat: Vault tokens are now identity-based instead of anonymous #4327

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 [email protected]

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-intel · 2023-02-06T23:03:03Z

cmd/security-spiffe-token-provider/res/configuration.toml

+#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-intel · 2023-02-06T23:05:04Z

internal/security/common/tokenpolicy_test.go

 //
 // 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-intel · 2023-02-06T23:06:50Z

internal/security/fileprovider/provider.go

-		// 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-intel · 2023-02-07T21:58:09Z

internal/security/common/usermanager.go

+	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 <[email protected]>

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-intel · 2023-02-08T15:50:06Z

internal/security/fileprovider/provider_test.go

@@ -157,12 +157,15 @@ func TestNoDefaultsCustomPolicy(t *testing.T) {
 	mockAuthTokenLoader.On("Load", privilegedTokenPath).Return("fake-priv-token", nil)

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


remove the dead code

bnevis-i requested review from jim-wang-intel, lenny-goodell and farshidtz 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-intel reviewed Feb 6, 2023

View reviewed changes

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

jim-wang-intel reviewed Feb 7, 2023

View reviewed changes

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

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

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Vault tokens are now identity-based instead of anonymous #4327

feat: Vault tokens are now identity-based instead of anonymous #4327

bnevis-i commented Feb 6, 2023

jim-wang-intel Feb 6, 2023

bnevis-i Feb 6, 2023 •

edited

Loading

jim-wang-intel Feb 6, 2023

jim-wang-intel Feb 6, 2023

bnevis-i Feb 6, 2023

jim-wang-intel Feb 6, 2023

bnevis-i Feb 6, 2023 •

edited

Loading

jim-wang-intel Feb 6, 2023

bnevis-i commented Feb 7, 2023

jim-wang-intel Feb 7, 2023

bnevis-i Feb 8, 2023 •

edited

Loading

jim-wang-intel Feb 8, 2023

sonarqubecloud bot commented Feb 8, 2023

jim-wang-intel Feb 8, 2023

feat: Vault tokens are now identity-based instead of anonymous #4327

feat: Vault tokens are now identity-based instead of anonymous #4327

Conversation

bnevis-i commented Feb 6, 2023

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

Choose a reason for hiding this comment

bnevis-i Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnevis-i Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnevis-i commented Feb 7, 2023

Choose a reason for hiding this comment

bnevis-i Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 8, 2023

Choose a reason for hiding this comment

bnevis-i Feb 6, 2023 •

edited

Loading

bnevis-i Feb 6, 2023 •

edited

Loading

bnevis-i Feb 8, 2023 •

edited

Loading