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!: Change identity source for secrets-config proxy adduser/deluser #4389

Closed
wants to merge 2 commits into from
Closed

feat!: Change identity source for secrets-config proxy adduser/deluser #4389

wants to merge 2 commits into from

Conversation

bnevis-i
Copy link
Collaborator

BREAKING CHANGE: Remove secrets-config proxy jwt subcommand and change the adduser/deluser commands so that they create vault identities and logins in Vault instead of creating them in Kong. This change will make it impossible to create a JWT that works with Kong proxy, and will break security-enabled TAF tests.

This is the third commit in a sequence of commits to implement the microservice authentication (token-based) ADR.

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?) See test script below
  • I have opened a PR for the related docs change (if not, why?) Documentation to be completed later.

Testing Instructions

Install the following script and run it to create a user. It is self testing, so if it outputs a JWT then it succeeded.

username=edgexuser

# Start afresh by deleting old user
docker exec -ti edgex-security-proxy-setup ./secrets-config proxy deluser --user "${username}" > /dev/null

# Create new user, log in, and exchange for JWT
password=$(docker exec -ti edgex-security-proxy-setup ./secrets-config proxy adduser --user "${username}" | jq -r '.password')
vault_token=$(curl -ks "http://localhost:8200/v1/auth/userpass/login/${username}" -d "{\"password\":\"${password}\"}" | jq -r '.auth.client_token')
id_token=$(curl -ks -H "Authorization: Bearer ${vault_token}" "http://localhost:8200/v1/identity/oidc/token/${username}" | jq -r '.data.token')

# Check that we got sane output from the previous commands before coughing up the token
introspect_result=$(curl -ks -H "Authorization: Bearer ${vault_token}" "http://localhost:8200/v1/identity/oidc/introspect" -d "{\"token\":\"${id_token}\"}" | jq -r '.active')
if [ "${introspect_result}" = "true" ]; then
	echo "${id_token}"
	exit 0
else
	echo "ERROR" >&2
	exit 1
fi

New Dependency Instructions (If applicable)

Copy link
Collaborator Author

@bnevis-i bnevis-i left a comment

Choose a reason for hiding this comment

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

Note to reviewers: Since this is a significant rewrite to the proxy subcommands, it is probably best to view the changed files in their entirety rather than try to make sense of the difference.

@bnevis-i bnevis-i added this to the Minnesota milestone Feb 23, 2023
@bnevis-i bnevis-i added this to QA/Code Review in Security WG Feb 23, 2023
@bnevis-i bnevis-i marked this pull request as draft February 23, 2023 03:20
@bnevis-i
Copy link
Collaborator Author

Need to remove unit test specific to the snap:

=== RUN   TestTLSCert
    proxy_test.go:24: Generate CA and server certificates
    exec.go:19: [exec] openssl ecparam -name prime256v1 -genkey -noout -out ./tmp/ca.key
    exec.go:19: [exec] openssl req -new -x509 -sha256 -key ./tmp/ca.key -out ./tmp/ca.crt -subj '/CN=snap-testing-ca'
    exec.go:19: [exec] openssl ecparam -name prime256v1 -genkey -noout -out ./tmp/server.key
    exec.go:19: [exec] openssl req -new -sha256 -key ./tmp/server.key -out ./tmp/server.csr -subj '/CN=localhost'
    exec.go:19: [exec] openssl x509 -req -in ./tmp/server.csr -CA ./tmp/ca.crt -CAkey ./tmp/ca.key -CAcreateserial -out ./tmp/server.crt -days 1 -sha256
    proxy_test.go:32: Add the self-signed certificate
    exec.go:19: [exec] sudo snap set edgexfoundry apps.secrets-config.proxy.tls.key='-----BEGIN EC PRIVATE KEY-----
        MHcCAQEEIOi2+eSm/3i3fLTxzqpTLPQ/uzVClbMCA0uWfXar0uyXoAoGCCqGSM49
        AwEHoUQDQgAExx6G78PIPbd9fhWv8t4XRDwLBmnNpXGOVpvmFFGaIv6w9+ye+Sxt
        aoYt3k+Jk+HCKWuJs+wMkNClKqLxczqlnQ==
        -----END EC PRIVATE KEY-----
        ' apps.secrets-config.proxy.tls.cert='-----BEGIN CERTIFICATE-----
        MIIBJzCBzwIUKNTWRXuHGxnEb+L+ZZCQV/PbSZAwCgYIKoZIzj0EAwIwGjEYMBYG
        A1UEAwwPc25hcC10ZXN0aW5nLWNhMB4XDTIzMDIyMzAzMDM1MFoXDTIzMDIyNDAz
        MDM1MFowFDESMBAGA1UEAwwJbG9jYWxob3N0MFkwEwYHKoZIzj0CAQYIKoZIzj0D
        AQcDQgAExx6G78PIPbd9fhWv8t4XRDwLBmnNpXGOVpvmFFGaIv6w9+ye+SxtaoYt
        3k+Jk+HCKWuJs+wMkNClKqLxczqlnTAKBggqhkjOPQQDAgNHADBEAiBG9s3Qukx4
        tYx3DCh84Kptakwb4cklLEhceYNQYUkw2wIgPCJ7AWbFqvs/cSTG2VUVIkBp7KPC
        SkW0R8B82gCQJBQ=
        -----END CERTIFICATE-----
        '
    exec.go:72: [stderr] error: cannot perform the following tasks:
        - Run service command "start" for services ["secrets-config-processor"] of snap "edgexfoundry" (systemctl command [start snap.edgexfoundry.secrets-config-processor.service] failed with exit status 1: Job for snap.edgexfoundry.secrets-config-processor.service failed because the control process exited with error code.
        See "systemctl status snap.edgexfoundry.secrets-config-processor.service" and "journalctl -xeu snap.edgexfoundry.secrets-config-processor.service" for details.
        )
    exec.go:75: exit status 1
    exec.go:19: [exec] sudo snap unset edgexfoundry apps
--- FAIL: TestTLSCert (1.11s)

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

Codecov Report

Merging #4389 (4282765) into main (248dcba) will decrease coverage by 0.91%.
The diff coverage is 60.65%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #4389      +/-   ##
==========================================
- Coverage   43.71%   42.80%   -0.91%     
==========================================
  Files         116      115       -1     
  Lines       10712    10428     -284     
==========================================
- Hits         4683     4464     -219     
+ Misses       5609     5563      -46     
+ Partials      420      401      -19     
Impacted Files Coverage Δ
internal/security/config/command/help/command.go 100.00% <ø> (ø)
...l/security/config/command/proxy/deluser/command.go 71.87% <50.00%> (-1.10%) ⬇️
...ernal/security/config/command/proxy/tls/command.go 76.00% <64.28%> (-9.55%) ⬇️
...l/security/config/command/proxy/adduser/command.go 69.44% <64.70%> (+5.61%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bnevis-i
Copy link
Collaborator Author

@MonicaisHer

Please help merge canonical/edgex-snap-testing#160 in order to remove the roadblock for this PR.

BREAKING CHANGE: Remove secrets-config proxy jwt subcommand
and change the adduser/deluser commands so that they
create vault identities and logins in Vault
instead of creating them in Kong. This change will make it
impossible to create a JWT that works with Kong proxy,
and will break security-enabled TAF tests.

This is the third commit in a sequence of commits to
implement the microservice authentication (token-based) ADR.

Signed-off-by: Bryon Nevis <bryon.nevis@intel.com>
Signed-off-by: Bryon Nevis <bryon.nevis@intel.com>
@sonarcloud
Copy link

sonarcloud bot commented Feb 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
5.5% 5.5% Duplication

@bnevis-i
Copy link
Collaborator Author

bnevis-i commented Mar 1, 2023

Closing - review #4244 instead.

@bnevis-i bnevis-i closed this Mar 1, 2023
Security WG automation moved this from QA/Code Review to Done Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Security WG
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants