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

Implement delegate server: GetKeySet #893

Merged
merged 9 commits into from Jan 3, 2018

Conversation

Projects
None yet
3 participants
@gdbelvin
Collaborator

gdbelvin commented Dec 21, 2017

No description provided.

@gdbelvin gdbelvin requested a review from phad Jan 2, 2018

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jan 2, 2018

Codecov Report

Merging #893 into master will increase coverage by 0.52%.
The diff coverage is 74.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
+ Coverage   47.45%   47.98%   +0.52%     
==========================================
  Files          33       34       +1     
  Lines        2381     2428      +47     
==========================================
+ Hits         1130     1165      +35     
- Misses       1053     1059       +6     
- Partials      198      204       +6
Impacted Files Coverage Δ
impl/sql/keysets/keysets.go 74.46% <74.46%> (ø)

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 053df4c...f9e3df0. Read the comment docs.

codecov-io commented Jan 2, 2018

Codecov Report

Merging #893 into master will increase coverage by 0.52%.
The diff coverage is 74.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
+ Coverage   47.45%   47.98%   +0.52%     
==========================================
  Files          33       34       +1     
  Lines        2381     2428      +47     
==========================================
+ Hits         1130     1165      +35     
- Misses       1053     1059       +6     
- Partials      198      204       +6
Impacted Files Coverage Δ
impl/sql/keysets/keysets.go 74.46% <74.46%> (ø)

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 053df4c...f9e3df0. Read the comment docs.

Show outdated Hide outdated core/fake/usermanager.go Outdated
Show outdated Hide outdated core/fake/usermanager.go Outdated
Show outdated Hide outdated core/fake/usermanager.go Outdated
Show outdated Hide outdated core/fake/usermanager.go Outdated
if err != nil {
return nil, err
}
ks.SigningKeys = nil // DON'T LEAK PRIVATE KEYS!!

This comment has been minimized.

@phad

phad Jan 2, 2018

Contributor

Great comment! This is all fine as long as the comment remains and there's just this simple single path through the func to something being returned.

I wonder if instead the sanitization could be done within storage.KeySets.Get().

You'd need an additional API though, as presumably some legitimate Get() calls do want to retrieve the private key. The additional API could have a much more explicit name like GetWithPrivate() or something.

@phad

phad Jan 2, 2018

Contributor

Great comment! This is all fine as long as the comment remains and there's just this simple single path through the func to something being returned.

I wonder if instead the sanitization could be done within storage.KeySets.Get().

You'd need an additional API though, as presumably some legitimate Get() calls do want to retrieve the private key. The additional API could have a much more explicit name like GetWithPrivate() or something.

This comment has been minimized.

@gdbelvin

gdbelvin Jan 2, 2018

Collaborator

I'm hoping to replace KeySets with Tink, which should remove this particular problem.

@gdbelvin

gdbelvin Jan 2, 2018

Collaborator

I'm hoping to replace KeySets with Tink, which should remove this particular problem.

Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
"github.com/google/keytransparency/impl/sql/engine"
"github.com/google/keytransparency/impl/sql/keysets"
"github.com/golang/glog"

This comment has been minimized.

@phad

phad Jan 2, 2018

Contributor

This, as well as "log" ?

@phad

phad Jan 2, 2018

Contributor

This, as well as "log" ?

This comment has been minimized.

@gdbelvin

gdbelvin Jan 2, 2018

Collaborator

Removed

@gdbelvin

gdbelvin Jan 2, 2018

Collaborator

Removed

Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated

@gdbelvin gdbelvin added the feature label Jan 2, 2018

@phad

phad approved these changes Jan 3, 2018

A few more typos but otherwise LGTM.

Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
Show outdated Hide outdated cmd/keytransparency-delegate/main.go Outdated
Show outdated Hide outdated impl/sql/keysets/keysets.go Outdated

gdbelvin added some commits Jan 3, 2018

@gdbelvin gdbelvin merged commit 4db6fe2 into google:master Jan 3, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gdbelvin gdbelvin deleted the gdbelvin:f/usermanager/getkeyset branch Jan 3, 2018

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