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

Remove secrets from Docker Images #1191

Merged
merged 9 commits into from Jul 19, 2019

Conversation

@gdbelvin
Copy link
Collaborator

commented Feb 7, 2019

Private keys should not be part of the docker images.
This was an old hack that predated docker's secrets feature.

TODO / Help wanted: add a kubernetes configmap that does the same thing.

@gdbelvin gdbelvin requested a review from RJPercival Feb 7, 2019

@gdbelvin gdbelvin force-pushed the gdbelvin:genkeys branch 2 times, most recently from 8f01e17 to 52b9c84 Feb 7, 2019

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 7, 2019

@DazWilkin I also verified that in this configuration, docker-compose up works successfully. Can you verify with respect to #1190 ?

@gdbelvin gdbelvin requested a review from DazWilkin Feb 7, 2019

@codecov

This comment has been minimized.

Copy link

commented Feb 7, 2019

Codecov Report

Merging #1191 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1191      +/-   ##
=========================================
- Coverage   30.33%   30.3%   -0.03%     
=========================================
  Files          48      48              
  Lines        3867    3867              
=========================================
- Hits         1173    1172       -1     
- Misses       2512    2513       +1     
  Partials      182     182
Impacted Files Coverage Δ
core/client/client.go 28.28% <0%> (-0.66%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c05089...1e210f7. Read the comment docs.

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Checking it now.

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

sequencer service needs the secret references too.

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Otherwise, it appears to work. I continue to have issues with context deadline exceeded

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 7, 2019

Just added the keys to the sequencer. See if that doesn't solve the context deadline exceeded issue...
If the sequencer isn't starting because it doesn't have access to those keys, it would prevent sequencing progress from happening which would cause a context exceeded error

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

I'd added they keys myself to get it to work but it doesn't address my issue :-(

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 7, 2019

The other place to look for these context deadline errors is the trillian log sequencer. If either the KT sequencer or the log sequencer are having issues clients will see deadline exceeded.

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

checking

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Everything else appears (!?) to be OK.

The only other recurring log entry that I see across the services is in server and it's:
interceptor.go:38] auth interceptor: no hander for /google.keytransparency.v1.KeyTransparency/GetRevision

NB Typo in hander sb handler

But, otherwise, everything appears OK. Should I publish my logs?

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

I have been unclear as to whether I should use 0.0.0.0:8080 (sequencer) or 0.0.0.0:443 (server).

Trying again using :443 for both authorized-keys create-keyset and then...

keytransparency-client post dazwilkin@google.com \
--client-secret=./client_secret.190205.json \
--insecure \
--data='dGVzdA==' \
--password=${PASSWORD} \
--kt-url=0.0.0.0:443 \
--verbose \
--logtostderr
Go to the following link in your browser then type the authorization code: 
https://accounts.google.com/o/oauth2/auth?access_type=online&client_id=[[REDACTED]]
[[REDACTED]]
2019/02/07 10:59:56 ✓ Signed Map Head signature verified.
2019/02/07 10:59:56 ✓ Log inclusion proof verified.
I0207 10:59:56.987019  158923 client.go:145] Trusted root updated to TreeSize 1
2019/02/07 10:59:56 ✓ Log root updated.
2019/02/07 10:59:56 ✓ Commitment verified.
2019/02/07 10:59:57 ✓ VRF verified.
2019/02/07 10:59:57 ✓ map inclusion proof verified.
2019/02/07 10:59:57 Got current entry...
2019/02/07 10:59:57 Sending Update request...
2019/02/07 10:59:58 ✓ Signed Map Head signature verified.
2019/02/07 10:59:58 ✓ Log inclusion proof verified.
Error: update failed: context deadline exceeded
@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 7, 2019

That timeout error is happening really quickly. I think the client is picking up an odd default timeout value of 0s for some reason.

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 7, 2019

The other thing that is happening is that we're starting the timeout counter before the user goes through the OAuth flow

@DazWilkin

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

So, you're telling me that my 'open link', allow, copy-paste then enter is too slow? ;-)

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 7, 2019

It's embarrassing :-) Fix: #1195

@gdbelvin gdbelvin added the security label Feb 8, 2019

@RJPercival

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

This isn't an issue introduced by this PR, but shouldn't the key generation scripts create and chmod the files before writing the keys to them?

gdbelvin added some commits Feb 6, 2019

Use $(go env GOPATH)
Use go env GOPATH for situations where the GOPATH environment variable may be unset
https://golang.org/cmd/go/#hdr-GOPATH_environment_variable

@gdbelvin gdbelvin force-pushed the gdbelvin:genkeys branch from 11aa66f to e494bd5 Feb 8, 2019

@NeverTestGitHub
Copy link

left a comment

google vrp test

mkdir -p "${GOPATH}/src/github.com/google/keytransparency/genfiles"
cd "${GOPATH}/src/github.com/google/keytransparency/genfiles"
mkdir -p "$(go env GOPATH)/src/github.com/google/keytransparency/genfiles"
cd "$(go env GOPATH)/src/github.com/google/keytransparency/genfiles"

This comment has been minimized.

Copy link
@RJPercival

RJPercival May 13, 2019

Member

super-nit: Won't work as expected if $GOPATH contains more than one path

@@ -23,7 +23,7 @@ while getopts d:a:s: option; do
d) COMMONNAME=${OPTARG};;
a) ADDRESS=${OPTARG};;
s) SAN_DNS=${OPTARG};;
*) echo "usage: ./generate.sh -d <domain> -a <ip_address> -s <san_extension_DNS>"; exit 1;;
*) echo "usage: ./gen_server_keys.sh -d <domain> -a <ip_address> -s <san_extension_DNS>"; exit 1;;

This comment has been minimized.

Copy link
@RJPercival

RJPercival May 13, 2019

Member

nit: Can use $0 or $(basename $0) instead of the filename here.

Merge branch 'master' into genkeys
* master: (106 commits)
  Remove unused logVerifier (#1324)
  Verify Revisions in StreamRevisions (#1323)
  Pair verifier functions (#1322)
  Split VerifyRevision into Verify{LogRoot,MapRevision (#1318)
  Make Previous hash check optional (#1307)
  Remove VerifySignedMapRoot from VerifierInterface (#1320)
  Remove trailing whitespace (#1321)
  Encapsulate Client Verifier State in test vectors (#1316)
  Pass along err message (#1314)
  Remove unnessesary func() (#1319)
  New test vector transcript format (#1315)
  Track map revision inside mutation (#1310)
  Move verifier to its own package (#1312)
  go generate ./... (#1306)
  Fix proto copying in revisions and paginator tests. (#1309)
  Fix proto copying in server_test. (#1308)
  go mod tidy (#1305)
  Use new TrillianMapWrite API (#1304)
  Configurable maximum queue depth for metric reporting. (#1303)
  Proposal to refine docker deployment (#1302)
  ...

@gdbelvin gdbelvin requested a review from google/keytransparency as a code owner Jul 18, 2019

@googlebot googlebot added the cla: yes label Jul 18, 2019

gdbelvin added some commits Jul 19, 2019

@gdbelvin gdbelvin merged commit 2aa29cd into google:master Jul 19, 2019

4 of 5 checks passed

codecov/project 30.3% (-0.03%) compared to 3c05089
Details
GolangCI No issues found!
Details
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
codecov/patch Coverage not affected when comparing 3c05089...1e210f7
Details

@gdbelvin gdbelvin deleted the gdbelvin:genkeys branch Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.