Conversation
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
I recommend performing a one-time transform of the input into something simpler to parse, like a JSON list.
Also, I think the WFE doesn't need to check for weak keys, so we can leave out the config for it, and somewhat simplify the instantiation. The main reason we call into goodkey in the WFE is to avoid trying to verify JWS signed by large RSA keys.
|
|
||
| func (wk *weakKeys) addSuffix(str string) error { | ||
| var suffix [10]byte | ||
| decoded, err := hex.DecodeString(str) |
There was a problem hiding this comment.
Since we assume these are all exactly 10 bytes decoded, we should check that here.
| @@ -0,0 +1,59 @@ | |||
| package goodkey | |||
There was a problem hiding this comment.
Add a file-level comment describing what this is for.
rolandshoemaker
force-pushed
the
new-weak-test
branch
from
767c631 to
7c816cd
Compare
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Few small nits but otherwise looking good! Thanks @rolandshoemaker
| for k := range flattened { | ||
| list = append(list, k) | ||
| } | ||
| jsonList, err := json.Marshal(list) |
There was a problem hiding this comment.
Should err be checked before calling ioutil.WriteFile? It seems like if marshalling fails the jsonList will be written anyway.
| return berrors.MalformedError("RSA keys are not allowed") | ||
| } | ||
| if policy.weakRSAList != nil { | ||
| if policy.weakRSAList.Known(key.N.Bytes()) { |
There was a problem hiding this comment.
Could you collapse these two if blocks into one with the condition policy.weakRSAList != nil && policy.weakRSAList.Known(key.N.Bytes()) ?
|
|
||
| wk, err := loadSuffixes(tempPath) | ||
| test.AssertNotError(t, err, "Failed to load suffixes from directory") | ||
|
|
| package goodkey | ||
|
|
||
| // This file defines a basic method for testing if a given RSA public key is on one of | ||
| // the Debian weak key lists and is therefore considered easily enumerable. Instead of |
There was a problem hiding this comment.
"...and the private key is therefore considered compromised" insead of "easily enumerable"
|
|
||
| // WeakKeyDirectory is the path to a directory containing truncated RSA modulus | ||
| // hashes of known easily enumerable keys. | ||
| WeakKeyDirectory string |
There was a problem hiding this comment.
This is now a file, not a directory.
| ) | ||
|
|
||
| func main() { | ||
| inputs := flag.String("inputs", "", "comma separated list of files to flatten") |
There was a problem hiding this comment.
Describe where to find this list of inputs?
| } | ||
| } | ||
|
|
||
| list := []string{} |
| } | ||
| jsonList, err := json.Marshal(list) | ||
| cmd.FailOnError(err, "failed to marshal list") | ||
| err = ioutil.WriteFile(*output, jsonList, os.ModePerm) |
There was a problem hiding this comment.
ModePerm is not right here; let's do 0640 instead.
| list = append(list, k) | ||
| } | ||
| jsonList, err := json.Marshal(list) | ||
| cmd.FailOnError(err, "failed to marshal list") |
There was a problem hiding this comment.
For FailOnError I've been trying to move to a style that eliminates the "failed", since it already logs that there is an error. E.g. cmd.FailOnError(err, "marshalling list")
| return nil, err | ||
| } | ||
|
|
||
| suffixList := []string{} |
| ) | ||
|
|
||
| type weakKeys struct { | ||
| suffixes map[[10]byte]struct{} |
There was a problem hiding this comment.
Let's define a type for [10]byte, e.g. type truncatedHash [10]byte
| for k := range flattened { | ||
| list = append(list, k) | ||
| } | ||
| jsonList, err := json.Marshal(list) |
There was a problem hiding this comment.
Let's use MarshalIndent here so we get an entry per line.
| // NewKeyPolicy returns a KeyPolicy that allows RSA, ECDSA256 and ECDSA384. | ||
| func NewKeyPolicy() KeyPolicy { | ||
| return KeyPolicy{ | ||
| func NewKeyPolicy(weakKeyDir string) (KeyPolicy, error) { |
There was a problem hiding this comment.
*weakKeyFile. Also, please add to the function comment describing the param, and the behavior when it's empty.
| return nil | ||
| } | ||
|
|
||
| func (wk *weakKeys) Known(modulus []byte) bool { |
There was a problem hiding this comment.
Nit: It's a little nicer for this to take an *rsa.PublicKey, and abstract away the fact that it really wants .N.Bytes(), which is an implementation detail.
| @@ -0,0 +1 @@ | |||
| ["0002a4226a4043426396","00008be7025d9f1a9088","0001313db46d8945bba0","00015b6662ff95aefa3f","000169a60c9eb82a558b","000220bb2bcbc060b8da","00024ac71844e42b0fa6","0002beb9288f6c0140cf","00008f7e6a29aea0b430","00026532237f74a48943","00006aa0ce2cd60e6660","00015e77627966ce16e7","00029956ea9997f257e1","0002a4ba3cf408927759"] No newline at end of file | |||
There was a problem hiding this comment.
Can you regenerate this with MarshalIndent so it's easier to read?
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
LGTM! Thanks @rolandshoemaker
Only the strong keys survive!
💪 🔑 💪
3 participants
Adds a basic truncated modulus hash check for RSA keys that can be used to check keys against the Debian
{openssl,openssh,openvpn}-blacklistlists of weak keys generated during the Debian weak key incident.Testing is gated on adding a new configuration key to the WFE, RA, and CA configs which contains the path to a directory which should contain the weak key lists.
Fixes #157.