Sanitize certificates from routes in the router#13897
Sanitize certificates from routes in the router#13897openshift-bot merged 1 commit intoopenshift:masterfrom
Conversation
|
[test] |
smarterclayton
force-pushed
the
sanitize_pem
branch
2 times, most recently
from
5df4170 to
8942e80
Compare
|
any comments? [test] |
|
I'm not familiar with the area, but I think @liggitt. Let's give him today. |
| return buf.Bytes() | ||
| } | ||
| // ignores blocks that can't be serialized | ||
| _ = pem.Encode(buf, block) |
There was a problem hiding this comment.
Open questions:
should we whitelist the PEM block types we allow ("CERTIFICATE", "PRIVATE KEY", "RSA PRIVATE KEY", etc)
When we're validating, are we making sure the data matches the PEM block types? Go's parsing sometimes ignores exact PEM block type matches and will parse any private key from any block type ending in "PRIVATE KEY"
which types does haproxy support?
Is haproxy forgiving about mismatched block type and block content?
|
Sanitization is a strict improvement, but the questions about haproxy type support and type/content mismatch are important to find out before we open up certs |
|
Haproxy just calls down to OpenSSL. So any block type supported which
is likely a huge list.
The mismatch between types is concerning - I can't think of a bounded
way to constrain that.
|
smarterclayton
force-pushed
the
sanitize_pem
branch
from
8942e80 to
80b0a31
Compare
|
Updated |
smarterclayton
force-pushed
the
sanitize_pem
branch
from
80b0a31 to
1bcaa48
Compare
|
|
||
| type blockVerifierFunc func(block *pem.Block) error | ||
|
|
||
| func publicKeyBlockVerifier(block *pem.Block) error { |
There was a problem hiding this comment.
modifying in-place is a little unexpected... I'd expect a new block returned. there are other bits (block headers, etc) that this isn't clearing
There was a problem hiding this comment.
should it clear them?
There was a problem hiding this comment.
ummm... not sure? the uses I've seen for PEM headers in these types of blocks are for encrypted private keys, which we don't support
| if block == nil { | ||
| return buf.Bytes(), nil | ||
| } | ||
| fn, ok := knownBlockDecoders[block.Type] |
There was a problem hiding this comment.
some ec private keys will include an EC PARAMETERS block that should be dropped
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm wondering if we should just drop unrecognized block types and continue (as long as we got at least one recognized block)
|
tests are a little light, a few nits, lgtm otherwise |
Ensure that no invalid PEM data is left in the route after extended validation passes.
smarterclayton
force-pushed
the
sanitize_pem
branch
from
1bcaa48 to
17abab0
Compare
|
Evaluated for origin test up to 17abab0 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1329/) (Base Commit: 2d26eae) |
|
more tests added. |
|
nits addressed |
|
LGTM |
|
[merge] |
1 similar comment
|
[merge]
|
|
[merge] install failure
…On Fri, May 12, 2017 at 12:37 PM, OpenShift Bot ***@***.***> wrote:
continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/618/)
(Base Commit: f24a57f
<f24a57f>
)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13897 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p8i34DAKeA94ExLvcNnBhyPQ3os9ks5r5IrdgaJpZM4NIGnK>
.
|
|
Evaluated for origin merge up to 17abab0 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/621/) (Base Commit: 4f2b9d7) (Image: devenv-rhel7_6225) |
4 participants
Ensure that no invalid PEM data is left in the route after extended
validation passes.
@liggitt