revoke: implement thread-safe compatibility and local file CRL #637
Conversation
Current coverage is 58.39% (diff: 62.06%)@@ master #637 diff @@
==========================================
Files 74 74
Lines 6404 6526 +122
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 3734 3811 +77
- Misses 2311 2350 +39
- Partials 359 365 +6
|
| log.Warningf("failed to parse CRL url: %v", err) | ||
| return false, false | ||
| } | ||
| if u.Scheme == "" && shouldFetchCRL { |
There was a problem hiding this comment.
Maybe this should use the file: scheme?
There was a problem hiding this comment.
file scheme doesn't work with ioutil.ReadFile. And do you really want to convert regular path to file:// and then convert it back for ReadFile?
There was a problem hiding this comment.
If you have a file:// URL, wouldn't the Path field work for the file name? RFC 5280 has a number of options for CRL distribution points, and the one that seems most relevant is a URI (other options include directory names, e.g. for LDAP, or nameRelativeToIssuer). It seems then that the file scheme is the most appropriate. If it's a local file, wouldn't it make sense to put it in CRLSet as a URI, not a plain path?
There was a problem hiding this comment.
I agree with @kisom, we need to be RFC compatible. We need to use URI here.
There was a problem hiding this comment.
@lziest RFC explains the certificate extension, and there is nothing about the local file. In our situation we set CRL path from the app and it doesn't relate to this RFC.
|
@kisom I also would like to remove the |
|
@kayrus I don't really have a problem with removing log except that it's useful for debugging --- if you have a good way around that, I'm all ears. I'm also fine with making |
kayrus
force-pushed
the
kayrus/local_crl
branch
7 times, most recently
from
7c425cc
to
da48568
Compare
|
@kisom added few improvements, could you please check? |
kayrus
force-pushed
the
kayrus/local_crl
branch
2 times, most recently
from
8b83384
to
5323aa7
Compare
| // CRLSet associates a PKIX certificate list with the URL the CRL is | ||
| // fetched from. | ||
| var CRLSet = map[string]*pkix.CertificateList{} | ||
| // NewRevokeChecker creates Revoke config structure |
There was a problem hiding this comment.
Per Go package conventions, this can just be New.
kayrus
force-pushed
the
kayrus/local_crl
branch
6 times, most recently
from
619e6a6
to
7a99f87
Compare
| var CRLSet = map[string]*pkix.CertificateList{} | ||
| // New creates Revoke config structure | ||
| func New() *Revoke { | ||
| return &Revoke{false, map[string]*pkix.CertificateList{}, nil} |
There was a problem hiding this comment.
prefer a named initialization: {
HardFail : false,
...
}
| t.Fatalf("expired certificate should have been marked as revoked") | ||
| } | ||
|
|
||
| insertLocalCRL(rc) |
There was a problem hiding this comment.
this insertLocalCRL and test again isn't giving any more line coverage, restore it to the original state.
|
Will push commits fixing the issues soon. |
|
@lziest please have a look once again. i've reworked lock logic. it is not optimized for remote URL fetching, but it is completely safe now. |
|
@lziest do you have any news on this PR? I'd like CRL stuff to be merged into k8s v1.4. This PR is a blocker. |
|
sorry, I don't have time to review it until next week. |
|
@lziest np. thanks for letting know. |
| } | ||
|
|
||
| if crl == nil { | ||
| return nil, fmt.Errorf("Local CRL is nil") |
There was a problem hiding this comment.
why return error with nil CRL? nil CRL is fine, right? It means empty list.
There was a problem hiding this comment.
ParseCRL returns ParseDERCRL (https://golang.org/src/crypto/x509/x509.go?s=51887:51948#L1647). ParseDERCRL returns crl=nil only when there is an error. I don't know whether new golang releases would return something else. That is why I added extra check. If you think I have to remove this check, so ok.
|
Let's start fresh. I will close this, and please re-submit with a separate PR. |
subj