feat(security): Replace security-proxy-setup for adding users #2808
Conversation
bnevis-i
added
enhancement
bnevis-i
requested review from
hutchic,
tonyespy,
jim-wang-intel and
lenny-goodell
Codecov Report
@@ Coverage Diff @@
## master #2808 +/- ##
==========================================
+ Coverage 39.52% 40.25% +0.73%
==========================================
Files 168 174 +6
Lines 13919 14255 +336
==========================================
+ Hits 5501 5739 +238
- Misses 8078 8140 +62
- Partials 340 376 +36
Continue to review full report at Codecov.
|
|
Detailed integration testing instructions added to PR description. |
|
@lenny-intel First round of review comments addressed. |
|
@bnevis-i , commit bot doesn't like |
Unfortunately, this behavior is mandated by git for --autosquash to work. Possible options are to It's nice to keep the commit history during the reviews, rather than force-pushing, as force pushing seems to break github's code comment hyperlinks. |
The bot is not in our control and sticks to strict conventional commit syntax. Option 3 is to follow conventional commits syntax and then squash the commits when merging. I agree the additional commits are useful during PR review. |
We could possibly make an upstream code contribution.... The code is quite simple and easy to extend. |
|
Rebased and squashed. |
bnevis-i
requested review from
tonyespy
and removed request for
tonyespy and
jim-wang-intel
cmd/edgex-secrets-config/README.md
Outdated
|
|
||
| * **adduser** | ||
|
|
||
| Create a API gateway user using specified token type. Requires additional arguments: |
There was a problem hiding this comment.
"Create a..." --> "Create an..."
| fmt.Println("TODO: Configure inbound TLS certificate.") | ||
| fmt.Printf("--in %s\n", c.certificatePath) | ||
| fmt.Printf("--inkey %s\n", c.privateKeyPath) | ||
| err = fmt.Errorf("tls command is unimplemented") |
There was a problem hiding this comment.
Ran out of time before code freeze.
There was a problem hiding this comment.
Maybe file an issue to track this once this PR lands?
There was a problem hiding this comment.
@bnevis-i thanks for all the changes.
Also I've attached a patch which adds the new cli to the snap, which would be nice if you could include in your PR. I only tested JWT, but everything works as expected if I run the command in a directory that contains a sub-directory named res which contains a configuration.toml file. My first attempt added a --confdir to the snapcraft.yaml app command, and that fails because the command expects "proxy " before any command-line options. So I stripped that out and tried to specify --confdir on the command-line, but each time I tried, the command complained about not being able to find ./res/configuration.toml. Note, I tried both --confdir and -confdir, also tried with and without the trailing /res.
espy@sumo$ edgexfoundry.edgex-secrets-config proxy deluser --user myuser --confdir /var/snap/edgexfoundry/current/config/edgex-secrets-config/res
level=ERROR ts=2020-10-30T17:29:35.890938657Z app=edgex-secrets-config source=bootstrap.go:45 msg="could not load configuration file (./res/configuration.toml): open ./res/configuration.toml: no such file or directory"
0001-build-snap-add-edgex-secrets-config-app.patch.txt
In addition there's a separate file (pipeline snap build optimization) which just needs to be copied (first you need to strip the .txt extension) over the existing patch file of the same name in /snap/local/patches.
cmd/edgex-secrets-config/README.md
Outdated
|
|
||
| * **--client\_id** _client\_id_ (required) | ||
|
|
||
| Optional manually-specified OAuth2 client_id. Will be generated if not present. Equivalent to a username. |
There was a problem hiding this comment.
So the usage says required, but then the text below says "Will be generated if not present". Which is it?
cmd/edgex-secrets-config/README.md
Outdated
|
|
||
| * **--client\_secret** _client\_secret_ (required) | ||
|
|
||
| Optional manually-specified OAuth2 client_secret. Will be generated if not present. Equivalent to a password. |
There was a problem hiding this comment.
So the usage says required, but then the text below says "Will be generated if not present". Which is it?
There was a problem hiding this comment.
Corrected this to say that it should have the values from the previous adduser command. Good catch.
| fmt.Println("TODO: Configure inbound TLS certificate.") | ||
| fmt.Printf("--in %s\n", c.certificatePath) | ||
| fmt.Printf("--inkey %s\n", c.privateKeyPath) | ||
| err = fmt.Errorf("tls command is unimplemented") |
There was a problem hiding this comment.
Maybe file an issue to track this once this PR lands?
| // Suppress log messages except errors as this utility | ||
| // outputs generated identifiers to stdout that we | ||
| // might want to capture via redirection | ||
| _ = lc.SetLogLevel("ERROR") |
There was a problem hiding this comment.
@lenny-intel can you answer my question as to whether or not this overrides LogLevel from configuration, and if so, why?
| // Suppress log messages except errors as this utility | ||
| // outputs generated identifiers to stdout that we | ||
| // might want to capture via redirection | ||
| _ = lc.SetLogLevel("ERROR") |
There was a problem hiding this comment.
It seems like this doesn't override the configuration LogLevel, as when I run the command from the snap, I see all of log messages (ERROR and INFO) written to stdout.
That said, the LogLevel in the configuration.toml still doesn't seem to work, as I tried setting it to ERROR there, and I still see all of the INFO messages:
[Writable]
LogLevel = "ERROR"
|
@tonyespy Lenny has fixed the log quietness issue. Can you let me know what the outstanding issues are. I'd like to get this one merged before Thanksgiving if possible. |
|
@tonyespy You are currently blocking the merge for this one. Can you let me know what the outstanding issues are? |
There was a problem hiding this comment.
The code changes all look good. I only had a few minor comments/questions inline.
One question about your test instructions. The first step is listed as:
make cmd/edgex-secrets-config/edgex-secrets-config
Did you actually mean cmd/edgex-secrets-config/res?
That said, when I built the snap from your branch and tried to test, the tool is panicking.
In my first attempt, since you're copying security-proxy-setup's configuration to $SNAP_DATA/config/secrets-config, I generated all of my certs & keys there. Then I ran the command without specifying --confdir:
$ cd /var/snap/edgexfoundry/current/config/secrets-config
$ edgexfoundry.secrets-config proxy adduser --token-type jwt --algorithm RS256 --public_key rsa4096.pub --user myuser
panic: interface conversion: interface {} is nil, not *config.ConfigurationStruct
goroutine 1 [running]:
github.com/edgexfoundry/edgex-go/internal/security/proxy/container.ConfigurationFrom(...)
/root/parts/edgex-go/src/internal/security/proxy/container/config.go:30
github.com/edgexfoundry/edgex-go/internal/security/config.(*Bootstrap).BootstrapHandler(0xc00014fd60, 0x829b40, 0xc000062600, 0xc00001c430, 0xbfe9c9d565d74520, 0x911f2, 0x9fbbe0, 0xdf8475800, 0x3b9aca00, 0xc00000c700, ...)
/root/parts/edgex-go/src/internal/security/config/bootstraphandler.go:36 +0x415
github.com/edgexfoundry/go-mod-bootstrap/bootstrap.RunAndReturnWaitGroup(0x829b40, 0xc000062600, 0xc000023230, 0x82c040, 0xc0000232c0, 0x7b5690, 0xe, 0x7b723e, 0x13, 0x82ad20, ...)
/root/go/pkg/mod/github.com/edgexfoundry/go-mod-bootstrap@v0.0.60/bootstrap/bootstrap.go:154 +0x629
github.com/edgexfoundry/go-mod-bootstrap/bootstrap.Run(0x829b40, 0xc000062600, 0xc000023230, 0x82c040, 0xc0000232c0, 0x7b5690, 0xe, 0x7b723e, 0x13, 0x82ad20, ...)
/root/go/pkg/mod/github.com/edgexfoundry/go-mod-bootstrap@v0.0.60/bootstrap/bootstrap.go:179 +0x149
github.com/edgexfoundry/edgex-go/internal/security/config.Main(0x829b40, 0xc000062600, 0xc000023230, 0xc000062600)
/root/parts/edgex-go/src/internal/security/config/main.go:51 +0x511
main.main()
/root/parts/edgex-go/src/cmd/secrets-config/main.go:18 +0x5b
Next, I tried the same command with the following permutations of --confdir, and all panic in the same way:
--confdir ./res--confdir .--confdir /var/snap/edgexfoundry/current/secrets-config/res
Any idea what's happening here?
|
@tonyespy Ready for re-review. Panic fixed. |
There was a problem hiding this comment.
No more panic, so progress!
That said, three comments inline.
Also a few other global comments:
- will there ever be a situation where you'd want different configuration files between security-proxy-setup and secrets-config?
- can you please add an issue to track implementation of the tls sub-command?
- I don't love the log formatted errors the command produces:
$ edgexfoundry.secrets-config proxy
level=ERROR ts=2020-12-02T20:07:23.613509623Z app=secrets-config source=bootstraphandler.go:69 msg="subcommand required (adduser, deluser, jwt, oauth2, tls)"
Not sure it's worth adding an issue to track improvements to this??
- And finally, your test instructions need minor tweaks (make cmd/secrets-config/secrets-config should be cmd/secrets-config/res? and get rid of the token type in your first deluser command).
| From: Tony Espy <espy@canonical.com> | ||
| Date: Wed, 28 Oct 2020 12:14:21 -0400 | ||
| Subject: [PATCH] [PATCH] optimize snap build for pipeline CI check | ||
| Date: Mon, 2 Nov 2020 16:07:09 -0500 |
There was a problem hiding this comment.
Your updated patch doesn't apply:
$ patch -p1 < ./snap/local/patches/0001-optimize-build-for-pipeline-CI-check.patch
patching file snap/snapcraft.yaml
Hunk #3 FAILED at 218.
Hunk #4 succeeded at 275 (offset 1 line).
Hunk #5 succeeded at 309 (offset 1 line).
Hunk #6 succeeded at 343 (offset 1 line).
Hunk #7 FAILED at 398.
2 out of 7 hunks FAILED -- saving rejects to file snap/snapcraft.yaml.rej
Hard to say. The plan is to put them into the same container for now. Eventually the bootstrapping ADR will probably result in refactoring of security-proxy-setup.
I don't think so.
I really do mean to make the executable here. I copy the res/configuration.toml from the other folder in the test instructions. |
Replaces security-proxy-setup for adding users to the Kong proxy. This new utility changes the JWT authentication mode from HS256 to RS256, thus avoiding exposure of the HMAC secret on the server side. This new utility also puts the client_id and client_secret for OAuth2 authentication in the hands of the user. Although in the new scheme the user is responsible for generating tokens via either the client_credentials OAuth2 flow, or client-side JWT signing, utility methods are provided for that function that do not depend upon the server. The new utility eliminates the requirement from security-proxy-setup that a valid security secret store token is available. Fixes #2477 Fixes #2549 Fixes #2568 Signed-off-by: Bryon Nevis <bryon.nevis@intel.com>
|
Kudos, SonarCloud Quality Gate passed!
|
…oundry#2808) Replaces security-proxy-setup for adding users to the Kong proxy. This new utility changes the JWT authentication mode from HS256 to RS256, thus avoiding exposure of the HMAC secret on the server side. This new utility also puts the client_id and client_secret for OAuth2 authentication in the hands of the user. Although in the new scheme the user is responsible for generating tokens via either the client_credentials OAuth2 flow, or client-side JWT signing, utility methods are provided for that function that do not depend upon the server. The new utility eliminates the requirement from security-proxy-setup that a valid security secret store token is available. Fixes edgexfoundry#2477 Fixes edgexfoundry#2549 Fixes edgexfoundry#2568 Signed-off-by: Bryon Nevis <bryon.nevis@intel.com>
…oundry#2808) Replaces security-proxy-setup for adding users to the Kong proxy. This new utility changes the JWT authentication mode from HS256 to RS256, thus avoiding exposure of the HMAC secret on the server side. This new utility also puts the client_id and client_secret for OAuth2 authentication in the hands of the user. Although in the new scheme the user is responsible for generating tokens via either the client_credentials OAuth2 flow, or client-side JWT signing, utility methods are provided for that function that do not depend upon the server. The new utility eliminates the requirement from security-proxy-setup that a valid security secret store token is available. Fixes edgexfoundry#2477 Fixes edgexfoundry#2549 Fixes edgexfoundry#2568 Signed-off-by: Bryon Nevis <bryon.nevis@intel.com>
Replaces security-proxy-setup for adding users to the Kong proxy.
This new utility changes the JWT authentication mode from HS256 to
RS256, thus avoiding exposure of the HMAC secret on the server side.
This new utility also puts the client_id and client_secret for
OAuth2 authentication in the hands of the user.
Although in the new scheme the user is responsible for generating
tokens via either the client_credentials OAuth2 flow,
or client-side JWT signing, utility methods are provided
for that function that do not depend upon the server.
The new utility eliminates the requirement from security-proxy-setup
that a valid security secret store token is available.
Signed-off-by: Bryon Nevis bryon.nevis@intel.com
PR Checklist
Please check if your PR fulfills the following requirements:
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-examples/blob/master/.github/CONTRIBUTING.md.
What is the current behavior?
Issue Number:
Fixes #2477
Fixes #2549
Fixes #2568
What is the new behavior?
Does this PR introduce a breaking change?
New Imports
Are there any new imports or modules? Ivi dof so, what are they used for and why?
Specific Instructions
Are there any specific instructions or things that should be known prior to reviewing?
Testing instructions (JWT)
Start up a minimal set of containers:
docker-compose -f docker-compose-nexus.yml up -d edgex-proxy metadata(Wait a few minutes for things to spin up.)
Do the following initial setup (after fetching the pull request branch)
Create a JWT user and test it (RSA):
Create a JWT user and test it (EC-DSA):
Shut it all down
Testing instructions (OAuth2)
First, edit docker-compose-nexus.yml and override the authentication algorithm to specify oauth2
Start up a minimal set of containers:
docker-compose -f docker-compose-nexus.yml up -d edgex-proxy metadata(Wait a few minutes for things to spin up.)
Create an oauth2 user and test it (use IDs returned from previous steps)
Shut it all down
Other information