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): Secure kong admin api #3328

Merged
merged 1 commit into from
May 4, 2021
Merged

feat(security): Secure kong admin api #3328

merged 1 commit into from
May 4, 2021

Conversation

beaufrusetta
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

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

Kong Admin API is exposed without authentication to the world.

Issue Number:

#2537

What is the new behavior?

Kong Admin API is secured with it's own key that security-proxy-setup uses to securely create the necessary services/routes.

Does this PR introduce a breaking change?

  • Yes
  • No

New Imports

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to reviewing?
There are some changes to edgex-compose that are necessary to complete this PR.

Other information

@beaufrusetta beaufrusetta changed the title Secure kong admin api feat(security): Secure kong admin api Mar 31, 2021
@hutchic hutchic linked an issue Mar 31, 2021 that may be closed by this pull request
cmd/security-proxy-setup/res/configuration.toml Outdated Show resolved Hide resolved
internal/security/bootstrapper/helper/helper.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Outdated Show resolved Hide resolved
internal/security/secretstore/init.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi.go Outdated Show resolved Hide resolved
internal/security/secretstore/kongadminapi_test.go Outdated Show resolved Hide resolved
cmd/security-secretstore-setup/res/configuration.toml Outdated Show resolved Hide resolved
@jim-wang-intel jim-wang-intel added 3-high priority denoting release-blocking issues ireland security-services labels Mar 31, 2021
@jim-wang-intel jim-wang-intel added this to the Ireland milestone Mar 31, 2021
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.

Review completed

jim-wang-intel
jim-wang-intel previously approved these changes Mar 31, 2021
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 from my side

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.

My issues have been addressed, will approve once @jim-wang-intel & @bnevis-i comments are fully addressed and approved.

@jim-wang-intel
Copy link
Contributor

My issues have been addressed, will approve once @jim-wang-intel & @bnevis-i comments are fully addressed and approved.

@beaufrusetta has addressed my comments, too so i'll leave it up to @bnevis-i to give the final go.

@beaufrusetta
Copy link
Contributor Author

beaufrusetta commented Mar 31, 2021

I've also just rebased my branch so that its even with master. @lenny-intel @jim-wang-intel @bnevis-i

@jim-wang-intel
Copy link
Contributor

I've also just rebased my branch so that its even with master. @lenny-intel @jim-wang-intel @bnevis-i

LGTM, and I think we need to squash it to the latest commit so that you can get over the Semantic Pull Request issue from CI pipeline.

@beaufrusetta
Copy link
Contributor Author

I've also just rebased my branch so that its even with master. @lenny-intel @jim-wang-intel @bnevis-i

LGTM, and I think we need to squash it to the latest commit so that you can get over the Semantic Pull Request issue from CI pipeline.

Agreed - talked to Lenny about that already. When @bnevis-i gets a chance to check over my updates and approve - I'll squash it and push it back up.

@lenny-intel @jim-wang-intel Also - do I need to officially document this anywhere? Are there some other tests that I could create? I mean, I literally just check to see if it throws an error or not. I don't necessarily have a very good "testing" mindset tbh. Suggestions?

@lenny-goodell
Copy link
Member

@lenny-intel @jim-wang-intel Also - do I need to officially document this anywhere? Are there some other tests that I could create? I mean, I literally just check to see if it throws an error or not. I don't necessarily have a very good "testing" mindset tbh. Suggestions?

This seems to all be what I would call Internals, so no user documentation that I see. Code should be self documenting for developers... ;-)

I didn't see anything else tests wise. @jim-wang-intel , @bnevis-i , Thoughts on additional testing?

@lenny-goodell
Copy link
Member

@beaufrusetta , you do have some failing tests that need to be addressed.

@jim-wang-intel
Copy link
Contributor

@lenny-intel @jim-wang-intel Also - do I need to officially document this anywhere? Are there some other tests that I could create? I mean, I literally just check to see if it throws an error or not. I don't necessarily have a very good "testing" mindset tbh. Suggestions?

This seems to all be what I would call Internals, so no user documentation that I see. Code should be self documenting for developers... ;-)

I didn't see anything else tests wise. @jim-wang-intel , @bnevis-i , Thoughts on additional testing?

@beaufrusetta, regarding the tests:
maybe add a couple of unit testing cases for GenerateRandomString(n)
not sure about how feasible you would unit test the Setup() in negative testing cases but yeah, if feasible, can add a couple of negative test cases for that.

@beaufrusetta
Copy link
Contributor Author

@lenny-intel @jim-wang-intel Also - do I need to officially document this anywhere? Are there some other tests that I could create? I mean, I literally just check to see if it throws an error or not. I don't necessarily have a very good "testing" mindset tbh. Suggestions?

This seems to all be what I would call Internals, so no user documentation that I see. Code should be self documenting for developers... ;-)
I didn't see anything else tests wise. @jim-wang-intel , @bnevis-i , Thoughts on additional testing?

@beaufrusetta, regarding the tests:
maybe add a couple of unit testing cases for GenerateRandomString(n)
not sure about how feasible you would unit test the Setup() in negative testing cases but yeah, if feasible, can add a couple of negative test cases for that.

Ok - I'll try to throw some things together and I'll post an update to the PR.

@beaufrusetta
Copy link
Contributor Author

@beaufrusetta , you do have some failing tests that need to be addressed.

Oh man...forgot about those tests. Ok - I'll address those. They all relate to setting up the services/routes/etc. I'll get them fixed...

bnevis-i
bnevis-i previously approved these changes Apr 1, 2021
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.

This looks like it has addressed all my comments and will be good once the test cases are passing.

Before checking this in, have we verified that the new functionality doesn't break cmd/secrets-config ?

@beaufrusetta
Copy link
Contributor Author

This looks like it has addressed all my comments and will be good once the test cases are passing.

Before checking this in, have we verified that the new functionality doesn't break cmd/secrets-config ?

@bnevis-i I'm working on that specifically as well...on my next check in, I'll call out what I've changed for a re-review.

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 suggestions of minor tweaks

internal/security/config/command/proxy/adduser/command.go Outdated Show resolved Hide resolved
internal/security/config/command/proxy/deluser/command.go Outdated Show resolved Hide resolved
internal/security/config/command/proxy/oauth2/command.go Outdated Show resolved Hide resolved
internal/security/proxy/service.go Outdated Show resolved Hide resolved
internal/security/config/command/proxy/adduser/command.go Outdated Show resolved Hide resolved
internal/security/config/command/proxy/adduser/command.go Outdated Show resolved Hide resolved
internal/security/proxy/service.go Outdated Show resolved Hide resolved
@beaufrusetta
Copy link
Contributor Author

@jim-wang-intel @lenny-intel @bnevis-i I've got all my stuff pushed - all appears to be working from my side - added back in the functions that I removed previously. Waiting on the CI test to see if there are any tests failing - and then I'll dive in to that.


// Does the config file path directory exist? If not, create it.
if !helper.CheckIfFileExists(k.paths.config) {
dirErr := os.MkdirAll(path.Dir(k.paths.config), 0644)
Copy link
Contributor

Choose a reason for hiding this comment

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

permission for directory should be 755 not 644,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? I feel like we changed it in a previous review from being more open to being more closed.


// Does the JWT file path directory exist? If not, create it.
if _, err := os.Stat(k.paths.jwt); os.IsNotExist(err) {
dirErr := os.MkdirAll(path.Dir(k.paths.jwt), 0644)
Copy link
Contributor

Choose a reason for hiding this comment

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

permission for directory should be 755 not 644,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? I feel like we changed it in a previous review from being more open to being more closed.

Copy link
Collaborator

@bnevis-i bnevis-i May 3, 2021

Choose a reason for hiding this comment

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

Directories need the execute bit set. e.g. 700, 750, 755. (which would be more restrictive than 700, 770, 777). Nobody does 700, 730, 733.

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 argument needed to be rectified

@bnevis-i
Copy link
Collaborator

bnevis-i commented May 3, 2021

Unit test failures.

lenny-goodell
lenny-goodell previously approved these changes May 3, 2021
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

Modified the secret-store process to create a declarative configuration file
for the Kong gateway configuration prior to start-up. The most logical place
for this configuration to take place was secretstore-setup, immediately after
vault was initialized.

To properly secure the Admin port, we first have to "hide" ports 8001/8444
by binding them to the Kong container. Doing that will only expose the ports
to the user that is running on the container --> kong. These changes will be
reflected in an adjoining PR on repo edgex-compose since they require
docker-compose changes.

Admin API access is needed by proxy-setup to create the services/routes to
the various microservices running in EdgeX. To enable that, a "loopback"
service/route combo was created that mapped "http://<host>:8000/admin" to
"http://<host>:8001". To avoid a chicken/egg problem, a declarative
configuration file is defined to import into Kong prior to starting the
Kong service for the first time. Values for an admin public key value and
JWT issuer value called out in the template are created and inserted.

The declarative configuration file enables plugins "JWT" and "ACL", and
creates a user (admin) and group (admin-group). All traffic to the "/admin"
path is locked down to the "admin-group" group and requires the use of a
Bearer token in the header of all requests to "/admin". A JWT is created
from an EC based public/private key pair, that is linked to the "admin"
user, and is written to the token path for proxy-setup.

Once the declarative configuration file is created with all necessary
values, it is written to "/tmp/kong/kong.yml" (changeable via config)
and is picked up by the Kong container through the use of the same
volume attachments between the 2 containers.

The Kong entrypoint script in bootstrap was modified to import the
declarative configuration file prior to "kong docker-start".

When Kong has been initialized, proxy-setup begins by reading in
the JWT file from its secure token directory, and then using that
JWT as the "Bearer" token via "Authorization" header in the API
requests to setup all of the gateway routes.

Closes: #2537

Signed-off-by: Beau Frusetta <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 4, 2021

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.8% 0.8% 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3-high priority denoting release-blocking issues ireland security-services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secure Kong admin port using password
4 participants