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(security)!: Implementation of JWT authentication ADR #4244

Merged
merged 2 commits into from
Mar 6, 2023
Merged

feat(security)!: Implementation of JWT authentication ADR #4244

merged 2 commits into from
Mar 6, 2023

Conversation

bnevis-i
Copy link
Collaborator

@bnevis-i bnevis-i commented Dec 3, 2022

BREAKING CHANGE: Requires JWT authentication for all inbound
requests except for /api/v2/ping URL. Removes support for
Kong reverse proxy. In place of Kong, uses NGINX
proxy auth module and introduces new security-prox-auth service.
Changes secrets-config proxy adduser/deluser commands to create
Vault users instead of Kong user. Changes secrets-config proxy tls
command to write TLS certificate to docker volume instead of Kong.
Removes security-proxy-setup go binary and replaces with shell
script to create default TLS token.

Signed-off-by: Bryon Nevis bryon.nevis@intel.com

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?)

Testing Instructions

New Dependency Instructions (If applicable)

@bnevis-i bnevis-i added this to In progress in Security WG Dec 3, 2022
@bnevis-i bnevis-i changed the title feat(wip): Create secret store tokens via identity auth feat(security)!: Implementation of JWT authentication ADR Feb 8, 2023
@bnevis-i bnevis-i marked this pull request as ready for review March 1, 2023 01:10
@bnevis-i bnevis-i added this to the Minnesota milestone Mar 1, 2023
@bnevis-i bnevis-i moved this from In progress to QA/Code Review in Security WG Mar 1, 2023
@bnevis-i
Copy link
Collaborator Author

bnevis-i commented Mar 1, 2023

@farshidtz To be unblocked by canonical/edgex-snap-testing#163

@farshidtz
Copy link
Member

@farshidtz To be unblocked by canonical/edgex-snap-testing#163

Thanks, looking into it. But please keep in mind that the snap tests don't block PR merges.

@bnevis-i
Copy link
Collaborator Author

bnevis-i commented Mar 1, 2023

@farshidtz To be unblocked by canonical/edgex-snap-testing#163

Thanks, looking into it. But please keep in mind that the snap tests don't block PR merges.

Thanks for the reminder. Probably ok to hold it then.

Copy link
Contributor

@jim-wang-intel jim-wang-intel left a comment

Choose a reason for hiding this comment

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

some minor changes

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Merging #4244 (32aa1ba) into main (a219a96) will decrease coverage by 1.89%.
The diff coverage is 60.00%.

📣 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    #4244      +/-   ##
==========================================
- Coverage   43.66%   41.78%   -1.89%     
==========================================
  Files         116      106      -10     
  Lines       10724     9736     -988     
==========================================
- Hits         4683     4068     -615     
+ Misses       5619     5321     -298     
+ Partials      422      347      -75     
Impacted Files Coverage Δ
...rnal/security/bootstrapper/command/flags_common.go 0.00% <ø> (ø)
internal/security/config/command/help/command.go 100.00% <ø> (ø)
internal/security/secretstore/init.go 14.35% <ø> (+0.22%) ⬆️
...rnal/security/secretstore/secretsengine/enabler.go 85.18% <0.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%) ⬆️
...al/security/bootstrapper/command/cmd_dispatcher.go 100.00% <100.00%> (ø)

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

BREAKING CHANGE: Requires JWT authentication for all inbound
requests except for /api/v2/ping URL. Removes support for
Kong reverse proxy.  In place of Kong, uses NGINX
proxy auth module and introduces new security-prox-auth service.
Changes secrets-config proxy adduser/deluser commands to create
Vault users instead of Kong user.  Changes secrets-config proxy tls
command to write TLS certificate to docker volume instead of Kong.
Removes security-proxy-setup go binary and replaces with shell
script to create default TLS token.

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

sonarcloud bot commented Mar 2, 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 2 Code Smells

No Coverage information No Coverage information
5.5% 5.5% Duplication

Copy link
Contributor

@jim-wang-intel jim-wang-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@cloudxxx8 cloudxxx8 requested review from chr1shung and removed request for chr1shung March 6, 2023 01:15
@bnevis-i bnevis-i merged commit 480818d into edgexfoundry:main Mar 6, 2023
Security WG automation moved this from QA/Code Review to Done Mar 6, 2023
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Already merged but I'd like to suggest making the CLI argument naming conventions consistent:

$ edgexfoundry.secrets-config proxy adduser -h
Usage of adduser:
  -configDir string
    	
  -jwtTTL string
    	JWT created by vault identity provider lasts this long (default "1h")
  -tokenTTL string
    	Vault token created as a result of vault login lasts this long (default "1h")
  -useRootToken
    	Set to true to TokenFile in config points to a resp-init.json instead of a service token
  -user string
    	Username of the user to add
level=ERROR ts=2023-03-10T11:55:04.114755814Z app=secrets-config source=bootstraphandler.go:70 msg="unable to parse command: -h: flag: help requested"

Ok - all camelCase

$ edgexfoundry.secrets-config proxy deluser -h
Usage of deluser:
  -configDir string
    	
  -useRootToken
    	Set to true to TokenFile in config points to a resp-init.json instead of a service token
  -user string
    	Username of the user to delete
level=ERROR ts=2023-03-10T11:55:10.795005034Z app=secrets-config source=bootstraphandler.go:70 msg="Unable to parse command: -h: flag: help requested"

Ok - all camelCase

$ edgexfoundry.secrets-config proxy tls -h
Usage of tls:
  -certfilename string
    	Filename of certificate file (on target) (default "nginx.crt")
  -configDir string
    	
  -incert string
    	Path to PEM-encoded leaf certificate
  -inkey string
    	Path to PEM-encoded private key
  -keyfilename string
    	Filename of private key file (on target (default "nginx.key")
  -targetfolder string
    	Path to TLS key file (default "/etc/ssl/nginx")
level=ERROR ts=2023-03-10T11:55:14.547332083Z app=secrets-config source=bootstraphandler.go:70 msg="unable to parse command: -h: flag: help requested"

Not OK.

  • -certfilename -> -certFilename
  • -incert -> -inCert
  • -inkey -> inKey
  • -keyfilename -> -keyFilename
  • -targetfolder -> -targetFolder

Also, I don't know why it is printing error on -h.

@bnevis-i I'll open an issue if necessary.

@bnevis-i bnevis-i deleted the vault-identity branch March 10, 2023 17:33
@bnevis-i
Copy link
Collaborator Author

@farshidtz See #4433 for fixes described in #4244 (review)

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

4 participants