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

Client Certificate Authentication with simple permissions model #1550

Merged
merged 20 commits into from Jan 9, 2019

Conversation

@jamesatheyDDS
Copy link
Contributor

jamesatheyDDS commented Jan 3, 2019

Description

Enables client certificate based authentication for the Orders Gateway and for DPS API.

  • Adds database table to contain known client certificates (by SHA-256 digest hash) with associated permissions
  • Dev and Test environments now trust the devlocal Certificate Authority
  • Documents how to generate certificates signed by the devlocal CA
  • Creates devlocal certificates for Air Force orders, Marine Corps orders, and DPS auth, and adds their hashes to the database in test and development
  • Checks for known client certificates in new middleware when accessing DPS or Orders APIs
  • Adds client certificate info to request Context, just like the user auth middleware does for the Session

Reviewer Notes

To add a partner's client cert to the database, get the SHA-256 hash of the digest, and the subject, and INSERT that as a row into the client_certs table, along with the desired permissions.

Setup

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in Querying the Database Safely have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.

References

jamesatheyDDS added some commits Dec 19, 2018

Basic client cert verification working
Use a fixed-length CHAR as the PRIMARY key in the client_certs table instead of a byte array - it's more natural for nearly everything, and performance should be just fine
Add devlocal-ca.pem to clientCAs in test and development
Create "faux" Air Force and Marine Corps certificates and keys, signed by the devlocal CA
Add the faux certificates to the database
Store client certificate information in context (like the Session is …
…for users)

Query the DB for the client certificate info in middleware instead of in certificate callback; revert changes to server.go and main.go for that
Check whether certificates are permitted to access APIs in Handlers for DPS Auth API and Orders Gateway API

@jamesatheyDDS jamesatheyDDS added the wip label Jan 3, 2019

@jamesatheyDDS jamesatheyDDS self-assigned this Jan 3, 2019

@jamesatheyDDS jamesatheyDDS changed the title Client Certificate Authentication with simple permissions model [WIP] [WIP] Client Certificate Authentication with simple permissions model Jan 3, 2019

jamesatheyDDS added some commits Jan 3, 2019

Add client_certs model unit tests
Add id UUID column to make _pop_ happy

@jamesatheyDDS jamesatheyDDS changed the title [WIP] Client Certificate Authentication with simple permissions model Client Certificate Authentication with simple permissions model Jan 7, 2019

@jamesatheyDDS jamesatheyDDS removed the wip label Jan 7, 2019

@jamesatheyDDS jamesatheyDDS referenced this pull request Jan 8, 2019

Open

[WIP] Orders Gateway #1396

0 of 11 tasks complete
Show resolved Hide resolved config/tls/README.md Outdated
Show resolved Hide resolved config/tls/README.md Outdated
@jamesatheyDDS

This comment has been minimized.

Copy link
Contributor

jamesatheyDDS commented Jan 8, 2019

@chrisgilmerproj I have added bin/generate-devlocal-cert.sh. It works from any directory and outputs a signed keypair.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

Some questions for you. Love the scripts you're writing too, so I'll be sure to come back and review again soon.

func ClientCertMiddleware(logger *zap.Logger, db *pop.Connection) func(next http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
mw := func(w http.ResponseWriter, r *http.Request) {
ctx, span := beeline.StartSpan(r.Context(), "ClientCertMiddleware")

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 8, 2019

Contributor

If the client cert is in the context are we going to see it in Honeycomb? Do we want it to be exposed there?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Jan 8, 2019

Contributor

My assumption is that honeycomb wouldn't extract that from the context by default, but worth testing on experimental before deploying to prod. We have beeline.StartSpan all over the codebase, so I don't know if this individual span is an issue.

This comment has been minimized.

@chrisgilmerproj
clientCert := authentication.ClientCertFromRequestContext(params.HTTPRequest)
if !clientCert.AllowOrdersAPI {
h.Logger().Info("Client certificate is not authorized to access this API")
return ordersoperations.NewGetOrdersUnauthorized()

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 8, 2019

Contributor

If all orders operations need this kind of authentication would it be possible to do this in a middleware step for the entire orders api? I just see it repeated several times below is why I ask.

This comment has been minimized.

@jamesatheyDDS

jamesatheyDDS Jan 9, 2019

Contributor

No, I don't think using the middleware for this will work. First of all, the client certificates are used for the DPS auth API as well, but that could be handled with separate instances of the middleware for the two APIs.

This is related to the question of whether boolean permissions per certificate will scale. There are a bunch of permissions specific to the Orders API that I expect it will need in the future, and these permissions checks would not look the same for each entrypoint. Some possible booleans in the future:

  • Read ( Army | Navy | Marines | AF | Coast Guard | Civilian ) orders
  • Modify ( Army | Navy | Marines | AF | Coast Guard | Civilian ) orders

And different certificates would have different sets of these booleans:

  • A branch of service's Orders system will have read / modify, but just for their branch.
  • A branch of service's analytics system will have read, but not modify, and just for their branch.
  • TRANSCOM's analytics might have read for all services, but not modify.
  • Very privileged admin users (using a CAC, possibly) would have read / modify for all.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 9, 2019

Contributor

That makes lots of sense to me. Thanks.

Show resolved Hide resolved pkg/models/client_cert.go
Subject string `db:"subject"`
AllowDpsAuthAPI bool `db:"allow_dps_auth_api"`
AllowOrdersAPI bool `db:"allow_orders_api"`
CreatedAt time.Time `db:"created_at"`

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Jan 8, 2019

Contributor

For AllowDpsAuthAPI and AllowOrdersAPI, why are we using booleans rather than role based authentication, where we assign a given user/cert a set of roles. I'm now sure the booleans will scale well, especially if we add admin interface, etc. While I don't want to overly complicate it more at this point.

This comment has been minimized.

@jamesatheyDDS

jamesatheyDDS Jan 9, 2019

Contributor

Using booleans here does not scale that far. I would prefer RBAC as well, but I wanted to keep the scope of this PR manageable. Doing RBAC would be a PR on its own, right? Being on the outside of your planning processes, I would not be comfortable designing the RBAC solution for MilMove.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

💯 - I'm good with this PR. I think there's still an outstanding question from Patrick you'll want to address first.

@jamesatheyDDS jamesatheyDDS merged commit c77c30d into master Jan 9, 2019

12 checks passed

ci/circleci: acceptance_tests Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details

@jamesatheyDDS jamesatheyDDS deleted the client-certs branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment