feat: Add optional capability to seed service secrets #276

lenny-goodell · 2021-09-22T21:08:13Z

Secrets are seeded from a JSON file specified by the SecretStore.SecretsFile setting
If SecretsFile setting is blank, seeding is skipped.

closes #273

This PR depends on edgexfoundry/go-mod-secrets#126 to be merged first.

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?)
feat: Document new Seeding Service Secrets capability edgex-docs#587

Testing Instructions

Start edgex using compose builder make run ds-camera
- This will create the SecretStore for device camera
Stop the Device Camera container
Clone branch for this PR and branch for go-mod-secrets PR feat: Add SecretsFile config setting go-mod-secrets#126
Add the following to device-sdk-go go.mod

replace (
	github.com/edgexfoundry/go-mod-bootstrap/v2 => ../go-mod-bootstrap
)

Add following the device-camera-go go.mod

replace (
	github.com/edgexfoundry/device-sdk-go/v2 => ../device-sdk-go
	github.com/edgexfoundry/go-mod-bootstrap/v2 => ../go-mod-bootstrap
)

Build device-camera
cd to cmd folder in device-camera-go

create file /tmp/camera-secrets.json with the following JSON:

{
    "secrets": [
        {
            "path": "credentials001",
            "imported": false,
            "secretData": [
                {
                    "key": "username",
                    "value": "my-user-1"
                },
                                {
                    "key": "password",
                    "value": "password-001"
                }
            ]
        }
    ]
}

run sudo EDGEX_SECURITY_SECRET_STORE=true SECRETSTORE_SECRETSFILE=/tmp/camera-secrets.json ./device-camera
Device Camera logs will have the following:

level=INFO ts=2021-09-27T18:22:23.1861784Z app=device-camera source=secret.go:52 msg="Creating SecretClient"
level=INFO ts=2021-09-27T18:22:23.1862282Z app=device-camera source=secret.go:59 msg="Reading secret store configuration and authentication token"
level=INFO ts=2021-09-27T18:22:23.1878451Z app=device-camera source=secret.go:71 msg="Attempting to create secret client"
level=INFO ts=2021-09-27T18:22:23.2092923Z app=device-camera source=secrets.go:277 msg="kick off token renewal with interval: 30m0s"
level=INFO ts=2021-09-27T18:22:23.209413Z app=device-camera source=secure.go:229 msg="Seeding 1 Service Secrets"
level=INFO ts=2021-09-27T18:22:23.2115168Z app=device-camera source=secure.go:248 msg="Secret for 'credentials001' successfully stored."
level=INFO ts=2021-09-27T18:22:23.2117384Z app=device-camera source=secure.go:218 msg="Scrubbing of secrets file complete."
level=INFO ts=2021-09-27T18:22:23.2117899Z app=device-camera source=secret.go:88 msg="Created SecretClient"

And will NOT display any error getting secrets, but will fail to initialize the camera since no camera exists.
9. The /tmp/camera-secrets.json file contents are now:

"secrets":[{"path":"credentials001","imported":true,"secretData":[]}]}

recreate /tmp/camera-secrets.json file contents as above with secret data.
run sudo EDGEX_SECURITY_SECRET_STORE=true SECRETSTORE_SECRETSFILE=/tmp/camera-secrets.json SECRETSTORE_DISABLESCRUBSECRETSFILE=true ./device-camera
Device Camera logs will have the following additional message:

level=INFO ts=2021-09-27T18:18:12.3870583Z app=device-camera source=secure.go:210 msg="Scrubbing of secrets file disable."

The /tmp/camera-secrets.json file contents will not be changed. I.e. secret data will still be present in the file

New Dependency Instructions (If applicable)

N/A

Secrets are seeded from a JSON file specifed by the SecretStore.SecretsFile setting If SecretsFile setting is blank, seeding is skipped. closes #273 Signed-off-by: Leonard Goodell <[email protected]>

siggiskulason · 2021-09-23T13:11:20Z

LGTM. Thanks for the testing instructions. I tested this with my ONVIF camera, the edgexfoundry snap and a natively running device-camera, setting the following in the camera configuration.toml file:

SecretsFile = "/home/sidar/tmp/camera-secrets.json"
TokenFile = "/var/snap/edgexfoundry/current/secrets/device-camera/secrets-token.json"

with camera-secrets.json being

{
    "secrets": [
        {
            "path": "credentials001",
            "imported": false,
            "secretData": [
                {
                    "key": "username",
                    "value": "admin"
                },
                                {
                    "key": "password",
                    "value": "admin"
                }
            ]
        }
    ]
}

and then running with

sudo EDGEX_SECURITY_SECRET_STORE=true ./device-camera

that works fine:

The secrets are seeded correctly
the JSON file gets overwritten as expected
The service authenticates with the camera and I see readings appear with images.

lenny-goodell · 2021-09-23T16:48:07Z

I tested this with my ONVIF camera, the edgexfoundry snap and a natively running device-camera,

@siggiskulason Excellent!! Thanks!

bootstrap/secret/secret.go

Signed-off-by: Leonard Goodell <[email protected]>

jim-wang-intel

LGTM

Signed-off-by: Leonard Goodell <[email protected]>

tonyespy

A couple of inline comments...

bootstrap/secret/secure.go

Signed-off-by: Leonard Goodell <[email protected]>

bnevis-i

Please tell us about the new dependency.

lenny-goodell · 2021-09-27T15:04:16Z

Please tell us about the new dependency.

@bnevis-i , it is new to this module, but not EdgeX. It is on the approved list here:
https://wiki.edgexfoundry.org/display/FA/Vetting+Process+for+3rd+Party+Dependencies

Signed-off-by: Leonard Goodell <[email protected]>

…ets file Signed-off-by: Leonard Goodell <[email protected]>

bootstrap/secret/secret.go

Signed-off-by: Leonard Goodell <[email protected]>

bnevis-i · 2021-09-27T19:57:14Z

bootstrap/secret/secret.go

+
+					lc.Debugf("SecretsFile is '%s'", secretConfig.SecretsFile)
+
+					if len(strings.TrimSpace(secretConfig.SecretsFile)) > 0 {


The code would be less complex if this was changed to if len == 0.

How? still an if/else just reversing the logic.

secureProvider.SetClient(secretClient) lc.Debugf("SecretsFile is '%s'", secretConfig.SecretsFile) if len(strings.TrimSpace(secretConfig.SecretsFile)) > 0 { err = secureProvider.LoadServiceSecrets(secretStoreConfig) if err != nil { return nil, err } } else { lc.Infof("SecretsFile not set, skipping seeding of service secrets.") } provider = secureProvider secureProvider.SetClient(secretClient) lc.Debugf("SecretsFile is '%s'", secretConfig.SecretsFile) if len(strings.TrimSpace(secretConfig.SecretsFile)) == 0 { lc.Infof("SecretsFile not set, skipping seeding of service secrets.") break } err = secureProvider.LoadServiceSecrets(secretStoreConfig) if err != nil { return nil, err } provider = secureProvider

I am always an advocate of early return to avoid indentation, just didn't see it here. THX!

Opps, not so fast. This skips the following important code when you do the break that you added, so still need the if/else of have repeated code.

provider = secureProvider lc.Info("Created SecretClient")

OK, was able to move that code up before handling of the secrets file and now the break you added will work.

bootstrap/secret/secure.go

jim-wang-intel

LGTM

Signed-off-by: Leonard Goodell <[email protected]>

tonyespy

I just want to clarify what's expected if the scrub fails, should the secrets provisioned into vault also be scrubbed? You also could structure the code such that the write to vault doesn't happen until after the JSON file gets scrubbed... If @bnevis-i you're OK with the way this is implemented, then I'll just approve, but as implemented this isn't being handled as a single transaction.

bnevis-i · 2021-09-28T13:48:52Z

I just want to clarify what's expected if the scrub fails, should the secrets provisioned into vault also be scrubbed? You also could structure the code such that the write to vault doesn't happen until after the JSON file gets scrubbed... If @bnevis-i you're OK with the way this is implemented, then I'll just approve, but as implemented this isn't being handled as a single transaction.

Yes, if the scrub fails the secrets will still be in the secret store. It is impossible to truly have a single transaction here as neither Vault nor the file system support a two-phase commit protocol. I'd rather have this behavior than the possibility that the JSON is scrubbed and then Vault fails, because there is no recovery from that.

lenny-goodell · 2021-09-28T15:19:31Z

I just want to clarify what's expected if the scrub fails, should the secrets provisioned into vault also be scrubbed? You also could structure the code such that the write to vault doesn't happen until after the JSON file gets scrubbed... If @bnevis-i you're OK with the way this is implemented, then I'll just approve, but as implemented this isn't being handled as a single transaction.

@tonyespy, The secrets do not need to be scrubbed from vault if file write fails. No harm in them still being there. Once write issue is resolved they will be over written. Trying to scrub them out of vault will add unneeded complexity.

* feat: Add optional capability to seed service secrets Secrets are seeded from a JSON file specified by the SecretStore.SecretsFile setting If SecretsFile setting is blank, seeding is skipped. * feat: Add DisableScrubSecretsFile setting to control scrubbing of secrets file closes edgexfoundry#273 Signed-off-by: Leonard Goodell <[email protected]>

lenny-goodell mentioned this pull request Sep 22, 2021

feat: Add SecretsFile config setting edgexfoundry/go-mod-secrets#126

Merged

5 tasks

lenny-goodell requested review from jim-wang-intel, bnevis-i and tonyespy September 22, 2021 21:34

feat: Add optional capability to seed service secrets

903b8e3

Secrets are seeded from a JSON file specifed by the SecretStore.SecretsFile setting If SecretsFile setting is blank, seeding is skipped. closes #273 Signed-off-by: Leonard Goodell <[email protected]>

lenny-goodell requested a review from siggiskulason September 22, 2021 21:43

siggiskulason approved these changes Sep 23, 2021

View reviewed changes

jim-wang-intel reviewed Sep 23, 2021

View reviewed changes

bootstrap/secret/secret.go Outdated Show resolved Hide resolved

fix: Addressed PR feeback

7df6da5

Signed-off-by: Leonard Goodell <[email protected]>

lenny-goodell force-pushed the secrets-file branch from c1ffab0 to 7df6da5 Compare September 23, 2021 18:09

lenny-goodell requested a review from jim-wang-intel September 23, 2021 18:10

jim-wang-intel approved these changes Sep 23, 2021

View reviewed changes

build: Updated go.mo for go-mod-secrets

59902fa

Signed-off-by: Leonard Goodell <[email protected]>

tonyespy reviewed Sep 23, 2021

View reviewed changes

bootstrap/secret/secure.go Outdated Show resolved Hide resolved

bootstrap/secret/secure.go Show resolved Hide resolved

bootstrap/secret/secure.go Outdated Show resolved Hide resolved

docs: Addressed PR review from Tony

366f197

Signed-off-by: Leonard Goodell <[email protected]>

lenny-goodell force-pushed the secrets-file branch from b81f151 to 366f197 Compare September 24, 2021 16:28

fix: update go.mod

63a0ae1

Signed-off-by: Leonard Goodell <[email protected]>

bnevis-i reviewed Sep 27, 2021

View reviewed changes

lenny-goodell mentioned this pull request Sep 27, 2021

feat: Document new Seeding Service Secrets capability edgexfoundry/edgex-docs#587

Merged

1 task

Leonard Goodell added 2 commits September 27, 2021 08:44

fix: ran GoFmt

b4238be

Signed-off-by: Leonard Goodell <[email protected]>

feat: Add DisableScrubSecretsFile setting to control srubbing of secr…

49d0fff

…ets file Signed-off-by: Leonard Goodell <[email protected]>

lenny-goodell requested review from tonyespy, bnevis-i, jim-wang-intel and siggiskulason September 27, 2021 18:27

jim-wang-intel reviewed Sep 27, 2021

View reviewed changes

bootstrap/secret/secret.go Outdated Show resolved Hide resolved

fix: Address PR feed back on parameter passing

69f7a6c

Signed-off-by: Leonard Goodell <[email protected]>

lenny-goodell requested a review from jim-wang-intel September 27, 2021 19:17

bnevis-i reviewed Sep 27, 2021

View reviewed changes

bnevis-i approved these changes Sep 27, 2021

View reviewed changes

jim-wang-intel approved these changes Sep 27, 2021

View reviewed changes

refactor: refactoring per PR comments

9366fe9

Signed-off-by: Leonard Goodell <[email protected]>

lenny-goodell requested review from bnevis-i and jim-wang-intel September 27, 2021 21:22

bnevis-i approved these changes Sep 27, 2021

View reviewed changes

jim-wang-intel approved these changes Sep 27, 2021

View reviewed changes

tonyespy reviewed Sep 28, 2021

View reviewed changes

tonyespy approved these changes Sep 28, 2021

View reviewed changes

lenny-goodell merged commit a4676a4 into edgexfoundry:main Sep 28, 2021

lenny-goodell deleted the secrets-file branch September 28, 2021 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add optional capability to seed service secrets #276

feat: Add optional capability to seed service secrets #276

lenny-goodell commented Sep 22, 2021 •

edited

Loading

siggiskulason commented Sep 23, 2021

lenny-goodell commented Sep 23, 2021

jim-wang-intel left a comment

tonyespy left a comment

bnevis-i left a comment

lenny-goodell commented Sep 27, 2021

bnevis-i Sep 27, 2021

lenny-goodell Sep 27, 2021

bnevis-i Sep 27, 2021 •

edited

Loading

lenny-goodell Sep 27, 2021

lenny-goodell Sep 27, 2021

lenny-goodell Sep 27, 2021

lenny-goodell Sep 27, 2021

lenny-goodell Sep 27, 2021

jim-wang-intel left a comment

tonyespy left a comment

bnevis-i commented Sep 28, 2021

lenny-goodell commented Sep 28, 2021


		lc.Debugf("SecretsFile is '%s'", secretConfig.SecretsFile)

		if len(strings.TrimSpace(secretConfig.SecretsFile)) > 0 {

feat: Add optional capability to seed service secrets #276

feat: Add optional capability to seed service secrets #276

Conversation

lenny-goodell commented Sep 22, 2021 • edited Loading

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

siggiskulason commented Sep 23, 2021

lenny-goodell commented Sep 23, 2021

jim-wang-intel left a comment

Choose a reason for hiding this comment

tonyespy left a comment

Choose a reason for hiding this comment

bnevis-i left a comment

Choose a reason for hiding this comment

lenny-goodell commented Sep 27, 2021

bnevis-i Sep 27, 2021

Choose a reason for hiding this comment

lenny-goodell Sep 27, 2021

Choose a reason for hiding this comment

bnevis-i Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

lenny-goodell Sep 27, 2021

Choose a reason for hiding this comment

lenny-goodell Sep 27, 2021

Choose a reason for hiding this comment

lenny-goodell Sep 27, 2021

Choose a reason for hiding this comment

lenny-goodell Sep 27, 2021

Choose a reason for hiding this comment

lenny-goodell Sep 27, 2021

Choose a reason for hiding this comment

jim-wang-intel left a comment

Choose a reason for hiding this comment

tonyespy left a comment

Choose a reason for hiding this comment

bnevis-i commented Sep 28, 2021

lenny-goodell commented Sep 28, 2021

lenny-goodell commented Sep 22, 2021 •

edited

Loading

bnevis-i Sep 27, 2021 •

edited

Loading