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

feat: Enable use of secrets via SecretProvider for MQTT broker credentials #197

Merged
merged 3 commits into from
Feb 3, 2021

Conversation

lenny-goodell
Copy link
Member

@lenny-goodell lenny-goodell commented Dec 31, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

MQTT broker credentials are in plain text configuration. No way to get them from SecretStore (aka Vault)

Issue Number: #159

What is the new behavior?

Via use of SpecretProvider, MQTT broker credentials can be pulled from a SecretStore. The SecretStore is Vault when running in secure mode or InsecureSecrets configuration section when running non-secure mode.

This PR is dependent on the following PRs:

edgexfoundry/go-mod-bootstrap#141
edgexfoundry/device-sdk-go#707

Does this PR introduce a breaking change?

  • Yes
  • No

Are there any new imports or modules? If so, what are they used for and why?

no

Are there any specific instructions or things that should be known prior to reviewing?

Do the following steps to test this PR:

  1. Cone developer-scripts
  2. cd to releases/nightly-build/compose-files
  3. Create password file and config file for MQTT
    • run mkdir mqtt-config
    • run echo 'edgex:$6$OV0KYbPEN4xbWLt9$MkEhwMRMZ8tTYvrlKHnUmZDJtzJGN1RcKNnoM1jNm7zzSgwQo9M0aAAB/8oTqCSQyVy1a42L7jO9xOsuNC9uhg==' > mqtt-config/passwords
    • run echo 'allow_anonymous false' > mqtt-config/mosquitto.conf
    • run echo 'password_file /mosquitto/config/passwords' >> mqtt-config/mosquitto.conf
  4. Add MQTT Broker
    • Add the following to nightly-build secure compose file (docker-compose-nexus.yml)
  mqtt-broker:
    container_name: edgex-mqtt-broker
    hostname: edgex-mqtt-broker
    image: eclipse-mosquitto:1.6.3
    networks:
      edgex-network: { }
    ports:
      - 1883:1883/tcp
    volumes:
    - ./mqtt-config:/mosquitto/config
  1. Add ADD_SECRETSTORE_TOKENS: "device-mqtt" to Vault Work in compose file.
  2. Run the compose file
    • make run
  3. MQTT Broker is now running with user/password auth
  • User is edge and Password is password
  • Use a tool like MQTT.fx to connect to verify the user and password are set correctly and are required.
  1. clone the branch for this PR
  2. Build Device MQTT
  3. cd cmd
  4. run sudo WRITABLE_LOGLEVEL=DEBUG EDGEX_SECURITY_SECRET_STORE=true ./device-mqtt -c ./res/example
  • Expect to see DEBUG message retrying Using Secrets URL of http://localhost:8200/v1/secret/edgex/device-mqtt/credentials`
  • Expect to see ERROR message like "Unable to retrieve MQTT credentials from SecretProvider at path 'credentials': Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Retrying for 9.9559743s"
    10 Store MQTT credentials
  • You have 60secs to store the MQTT credential via /api/v2/secret endpoint before service fails.
    • This can be increased in config via Driver.CredentialsRetryTime setting
  • While DEBUG messages are being logged send the following via REST POST to http://localhost:49982/api/v2/secret
{
  "path" : "credentials",
  "secretData" : [
    {
      "key" : "username",
      "value" : "edgex"
    },
	{
      "key" : "password",
      "value" : "password"
    }
  ]
}
 - Expected response it:
{
    "apiVersion": "v2",
    "requestId": "",
    "message": "",
    "statusCode": 201
}
  • The following messages will then be logged:
 "[Response listener] Start command response listening."
 "[Incoming listener] Start incoming data listening. "
  • Device Service is ready to accept async data from a device
  1. From a tool like MQTT.fx publish the following to the DataTopic topic
{
  "name" : "MQTT-test-device",
  "cmd" : "message",
  "message" : "Hellow World"
}
  • "SendEvent: Pushed event to core data" should be logged.
  1. Use core-data REST API to confirm reading received by sending GET to localhost:48080/api//v1/reading/name/message/0

Other information

@codecov-io
Copy link

codecov-io commented Jan 12, 2021

Codecov Report

Merging #197 (4ee44f0) into master (cdd6c33) will decrease coverage by 2.03%.
The diff coverage is 2.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   27.64%   25.60%   -2.04%     
==========================================
  Files           5        5              
  Lines         463      492      +29     
==========================================
- Hits          128      126       -2     
- Misses        321      352      +31     
  Partials       14       14              
Impacted Files Coverage Δ
internal/driver/driver.go 16.78% <0.00%> (-0.51%) ⬇️
internal/driver/incominglistener.go 0.00% <0.00%> (ø)
internal/driver/responselistener.go 0.00% <0.00%> (ø)
internal/driver/config.go 50.00% <5.00%> (-31.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdd6c33...4ee44f0. Read the comment docs.

@lenny-goodell lenny-goodell force-pushed the secrets branch 2 times, most recently from 4345886 to 4ee44f0 Compare January 12, 2021 22:22
Copy link
Member

@tonyespy tonyespy left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I followed your testing instructions and was able to get everything working as expected. That said, I did have a few minor comments line, some which need addressing.

tonyespy
tonyespy previously approved these changes Jan 26, 2021
Copy link
Member

@tonyespy tonyespy left a comment

Choose a reason for hiding this comment

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

Thanks for the changes/responses...

@lenny-goodell lenny-goodell marked this pull request as ready for review February 3, 2021 16:19
lenny added 3 commits February 3, 2021 09:19
dumb-init is needed when running the injected security bootstrapper entry point script for secure mode.

Signed-off-by: lenny <[email protected]>
Copy link
Member

@iain-anderson iain-anderson left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security credentials should come from Vault, not configuration.toml
4 participants