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.

Sign up for GitHub

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

Jump to bottom

feat(security): implement secrets-config proxy tls #2930

Merged
merged 5 commits into from
Dec 10, 2020
Merged

feat(security): implement secrets-config proxy tls #2930

merged 5 commits into from
Dec 10, 2020

Conversation

jim-wang-intel
Copy link
Contributor

Add implementation for secrets-config: proxy tls subcommand
It uploads the user-provided Kong TLS certificate/key pair to proxy server.

Closes: #2866
Signed-off-by: Jim Wang [email protected]

PR Checklist

Please check if your PR fulfills the following requirements:

  • [X ] Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/master/.github/Contributing.md.

What is the current behavior?

Issue Number: #2866

What is the new behavior?

Added new implementation of proxy tls into secrets-config to upload the kong tls cert

Does this PR introduce a breaking change?

  • Yes
  • [X ] No

New Imports

  • Yes
  • [X ] No

Specific Instructions

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

Other information

@jim-wang-intel jim-wang-intel added enhancement New feature or request request for comments security-services 3-high priority denoting release-blocking issues ireland labels Dec 7, 2020
@jim-wang-intel jim-wang-intel added this to the Ireland milestone Dec 7, 2020
@jim-wang-intel jim-wang-intel self-assigned this Dec 7, 2020
@codecov-io
Copy link

codecov-io commented Dec 7, 2020 •
edited
Loading

Codecov Report

Merging #2930 (a41decd) into master (34dce46) will increase coverage by 0.32%.
The diff coverage is 82.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2930      +/-   ##
==========================================
+ Coverage   40.31%   40.63%   +0.32%     
==========================================
  Files         170      170              
  Lines       14036    14156     +120     
==========================================
+ Hits         5658     5752      +94     
- Misses       8010     8026      +16     
- Partials      368      378      +10
Impacted Files Coverage Δ
...ernal/security/config/command/proxy/tls/command.go 84.39% <82.40%> (-15.61%) ⬇️
...l/security/config/command/proxy/adduser/command.go 58.62% <100.00%> (ø)
internal/core/data/v2/application/event.go 60.52% <0.00%> (-3.51%) ⬇️

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 34dce46...a41decd. Read the comment docs.

@bnevis-i
Copy link
Collaborator

bnevis-i commented Dec 8, 2020 •
edited
Loading

Ok. The code is uploading the cert but not making it active.

 openssl s_client -connect localhost:8443

should output the TLS cert that we uploaded. It is currently returning the default cert.

Looks like the SNIs parameter is required and it won't take '*'

@jim-wang-intel
feat(security): implement secrets-config proxy tls
dd9eef3 
Add implementation for secrets-config: proxy tls subcommand
It uploads the user-provided Kong TLS certificate/key pair to proxy server.

Closes: edgexfoundry#2866
Signed-off-by: Jim Wang <[email protected]>
@jim-wang-intel
feat(security): Implement secrets-configure proxy tls
aab1748 
Add SNIS association with the user specified certificates.
Also addressed PR feedback.

Signed-off-by: Jim Wang <[email protected]>
@jim-wang-intel jim-wang-intel force-pushed the implement-proxy-tls-secrets-config branch from 0b250cd to aab1748 Compare December 8, 2020 17:09
@jim-wang-intel
Copy link
Contributor Author

Ok. The code is uploading the cert but not making it active.

 openssl s_client -connect localhost:8443

should output the TLS cert that we uploaded. It is currently returning the default cert.

Looks like the SNIs parameter is required and it won't take '*'

Added SNIS association, now the command line: openssl s_client -servername localhost -connect localhost:8443 responds with the user-provided TLS cert.

@jim-wang-intel
Copy link
Contributor Author

To test this locally on docker-based Kong, do the following steps:

  1. git clone this PR
  2. build codes by make build
  3. copy the temporary res/ directory from security-proxy-setup's: 
        cp -r cmd/security-proxy-setup/res cmd/secrets-config/
  4. open another terminal and go to developer-scripts master branch and change directory to compose-builder
  5. run the edgex docker stack by make run
  6. wait for few seconds to let the whole stack up and running
  7. go back to the previous terminal that has edgex-go, and change directory to secrets-config: 
      cd cmd/secrets-config/
  8. run the executable proxy tls subcommand by: 
      ./secrets-config proxy tls --incert <Path_to_Your_Cert_In_Encoded_PEM_Format>yourcert.pem --inkey <Path_to_Your_Cert_Private_Key_In_PEM_Format>yourprivatekey
  9. You should see no error in the console prompt after you run that subcommand.
  10. You can also verify the certificate being used against the running edgex docker-stack via curl command or openssl provided by Bryon's:
   openssl s_client -servername localhost -connect localhost:8443

@bnevis-i
Copy link
Collaborator

bnevis-i commented Dec 8, 2020 •
edited
Loading

Please update README.md (in cmd/secrets-config folder) with new command line option.

@jim-wang-intel
docs(security): Update the secrets-config README
0f33407 
Add user's guide documentation for a new optional argument of proxy tls subcommand: --snis [list of names in comma separated]

Signed-off-by: Jim Wang <[email protected]>
@jim-wang-intel
feat(security): Change the list certificates algorithm
35953d6 
With Bryon's suggestion not using /certificates API, now it changes to /snis and only deletes those certificates associated with the snis names,
including the buitlin ones.

Signed-off-by: Jim Wang <[email protected]>
bnevis-i
bnevis-i previously approved these changes Dec 9, 2020
View reviewed changes
Copy link
Collaborator

@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.

LGTM

@jim-wang-intel
refactor(security): Use user-defined structs for json unmarshal
a41decd 
User-defined structs defined locally and are used to do json unmarshal so that only decoding once
Addressed per Lenny's request for changes.

Signed-off-by: Jim Wang <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 9, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.4% 0.4% Duplication

lenny-goodell
lenny-goodell approved these changes Dec 9, 2020
View reviewed changes
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

@lenny-goodell lenny-goodell merged commit 382321c into edgexfoundry:master Dec 10, 2020
jim-wang-intel added a commit to jim-wang-intel/edgex-go that referenced this pull request Dec 15, 2020
@jim-wang-intel
feat(security): implement secrets-config proxy tls (edgexfoundry#2930)
11e2716 
* feat(security): implement secrets-config proxy tls

Add implementation for secrets-config: proxy tls subcommand
It uploads the user-provided Kong TLS certificate/key pair to proxy server.

Closes: edgexfoundry#2866
Signed-off-by: Jim Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenny-goodell lenny-goodell lenny-goodell approved these changes

@bnevis-i bnevis-i bnevis-i left review comments

@beaufrusetta beaufrusetta Awaiting requested review from beaufrusetta

@hutchic hutchic Awaiting requested review from hutchic

@tonyespy tonyespy Awaiting requested review from tonyespy

@cloudxxx8 cloudxxx8 Awaiting requested review from cloudxxx8

Assignees

@jim-wang-intel jim-wang-intel

Labels
3-high priority denoting release-blocking issues enhancement New feature or request ireland request for comments security-services
Projects
None yet
Milestone
Ireland
Development

Successfully merging this pull request may close these issues.

Implement missing "proxy tls" functionality in edgex-secrets-config
4 participants
@jim-wang-intel @codecov-io @bnevis-i @lenny-goodell