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

Store and check previously denied CSRs #185

Merged
merged 6 commits into from
May 14, 2015

Conversation

rolandshoemaker
Copy link
Contributor

Fix for #101, I plan to add RA tests tomorrow.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 47.91% when pulling a2b3461 on rolandshoemaker:deny-store into fdc1825 on letsencrypt:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.94%) to 47.05% when pulling 0718250 on rolandshoemaker:deny-store into fdc1825 on letsencrypt:master.

@jsha
Copy link
Contributor

jsha commented May 12, 2015

I think storing whole CSRs is the wrong approach. I think we to store a list of DNS names, possibly with some extra metadata. Otherwise you could avoid the list by changing some unimportant part of the CSR.

Also one thing I didn't notice until re-reading the relevant CPS part is that this denied list is only necessary for revocations or denials "due to suspected phishing or other fraud." So we still need the table, but it can be pretty small.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.07%) to 46.92% when pulling 07e6f10 on rolandshoemaker:deny-store into fdc1825 on letsencrypt:master.

@rolandshoemaker
Copy link
Contributor Author

Ready for re-review. Not too sure about my method for passing a bool in rpc-wrappers.go so thoughts on that would be appreciated.

@jcjones
Copy link
Contributor

jcjones commented May 14, 2015

@rolandshoemaker I don't think there's anything in golang you can do to make that cleaner, except to marshal to/from JSON.

I'm fine with it as-is, or we can write a tiny marshalBool unmarshalBool function pair to be cleaner.

@jcjones
Copy link
Contributor

jcjones commented May 14, 2015

Oh, and LGTM, ready to commit when you are @rolandshoemaker

@rolandshoemaker
Copy link
Contributor Author

If we start passing more bools via RPC it'll make sense to write a function pair but for now i'm okay leaving it as-is...

jcjones added a commit that referenced this pull request May 14, 2015
Store and check previously denied CSRs
@jcjones jcjones merged commit 6be5c49 into letsencrypt:master May 14, 2015
@jcjones
Copy link
Contributor

jcjones commented May 14, 2015

Build failures merged to master:

wfe/web-front-end_test.go:105: cannot use MockSA literal (type *MockSA) as type core.StorageGetter in assignment:
    *MockSA does not implement core.StorageGetter (missing AlreadyDeniedCSR method)
wfe/web-front-end_test.go:270: cannot use MockSA literal (type *MockSA) as type core.StorageGetter in assignment:
    *MockSA does not implement core.StorageGetter (missing AlreadyDeniedCSR method)
wfe/web-front-end_test.go:331: cannot use MockSA literal (type *MockSA) as type core.StorageGetter in assignment:
    *MockSA does not implement core.StorageGetter (missing AlreadyDeniedCSR method)
wfe/web-front-end_test.go:420: cannot use MockSA literal (type *MockSA) as type core.StorageGetter in assignment:
    *MockSA does not implement core.StorageGetter (missing AlreadyDeniedCSR method)

jcjones pushed a commit that referenced this pull request May 14, 2015
jsha added a commit that referenced this pull request May 14, 2015
Add missing mock method to fix build for PR #185.
@jsha jsha mentioned this pull request May 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants