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

Check DOD certs on startup #1492

Merged
merged 1 commit into from Jan 8, 2019

Conversation

3 participants
@pjdufour-truss
Copy link
Contributor

pjdufour-truss commented Dec 19, 2018

Description

This PR makes a few minor improvements to DOD certificate management.

  • migrates from github.com/fullsailor/pkcs7 to go.mozilla.org/pkcs7
  • checks and parses DOD server keypair within webserver and passes tls.Certificate directly instead of using intermediate struct
  • Outputs additional debug information

Reviewer Notes

Is there anything you would like reviewers to give additional scrutiny?

DOD certs aren't currently checked in CI/CD, since they require real values. You can test with a
.envrc.local or load certs from chamber as below.

TEST_ACC_DOD_CERTIFICATES=1 TEST_ACC_ENV=experimental make webserver_test

Setup

No external changes

Code Review Verification Steps

References

@pjdufour-truss pjdufour-truss force-pushed the validate_certs branch 2 times, most recently from cde1b41 to a5f93c3 Dec 19, 2018

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

🚀 - Unclear why your deps aren't working on CircleCI. But I'd love to know what's up when you figure it out.

@@ -87,8 +80,6 @@ func (s Server) serverConfig(tlsConfig *tls.Config) (*http.Server, error) {

// tlsConfig generates a new *tls.Config. It will

This comment has been minimized.

@ntwyman

ntwyman Dec 21, 2018

Contributor

It will what?

"dod-ca-package",
}

for _, c := range pkcs7Vars {

This comment has been minimized.

@ntwyman

ntwyman Dec 21, 2018

Contributor

Why is this is a loop over a single entry?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Dec 21, 2018

Contributor

yeah, will take out the loop in this PR

return errors.Wrap(&errInvalidPKCS7{Path: pathToPackage}, fmt.Sprintf("%s is missing", c))
}

pkcs7Package, err := ioutil.ReadFile(pathToPackage) // #nosec

This comment has been minimized.

@ntwyman

ntwyman Dec 21, 2018

Contributor

Why #nosec?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Dec 21, 2018

Contributor

gosec throws and error whenever you read from an arbitrary path. Other than filepath.clean, which has no security change, is there a better way to ignore this warning?

@@ -3,7 +3,7 @@ package server
import (
"crypto/x509"

"github.com/fullsailor/pkcs7"
"go.mozilla.org/pkcs7"

This comment has been minimized.

@ntwyman

ntwyman Dec 21, 2018

Contributor

Why is this the right thing to do?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Dec 21, 2018

Contributor

the mozilla fork is maintained

return errors.Wrap(&errInvalidPKCS7{Path: pathToPackage}, fmt.Sprintf("%s is an empty file", c))
}

_, err = server.LoadCertPoolFromPkcs7Package(pkcs7Package)

This comment has been minimized.

@ntwyman

ntwyman Dec 21, 2018

Contributor

Why does it make sense to call this twice. Once where we use the result and once where we dont. In both cases we check the error so it just feels like duplicated cycles.

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Dec 21, 2018

Contributor

understood

@pjdufour-truss pjdufour-truss force-pushed the validate_certs branch 5 times, most recently from 097d7c1 to 2470039 Dec 21, 2018

@pjdufour-truss pjdufour-truss force-pushed the validate_certs branch from 2470039 to 663a1a0 Jan 7, 2019

@pjdufour-truss pjdufour-truss force-pushed the validate_certs branch from 542d642 to 468c396 Jan 7, 2019

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

LGTM

@ntwyman

ntwyman approved these changes Jan 8, 2019

@pjdufour-truss pjdufour-truss merged commit be537a9 into master Jan 8, 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

@pjdufour-truss pjdufour-truss deleted the validate_certs branch Jan 8, 2019

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