| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 632
Validate mail domains in the DNS correctly #1230
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
Conversation
|
Thanks very much for fixing this! Please add a unittest as well. |
ra/registration-authority.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this later percolates to VA.DNS.RTT.A (previously VA.DNS.RTT.MX). Not sure if something should be done about that or not.
Also, I think it could all be done in a single DNS query, but the code changes would be more invasive.
I guess this approach is great if it can be granted the General Availability tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this!
I haven't checked how the RTT is used; my guess was that it might be used for a timeout or something, so I thought it would be best to return the total elapsed time.
DNS queries can only ask for one type at a time, so you can't ask for MX and A and AAAA in one go. (ANY queries are magical and do not work the way you think they do.)
|
OK I have updated this PR with some deliciously wholesome tests, and Travis says the unit tests are working. The integration tests failed, but I don't think that's my fault... Give me a shout if there's anything else I need to fix with this. |
|
Yeah, we have a flaky integration test. Sorry. A re-run has it green now. |
|
LGTM |
|
Great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wasn't sure what to do with the RTTs - I was very lazy and didn't check what they are used for, so thanks for the pointer. I've pushed an updated patch which makes the RTT stats slightly less incorrect.
|
There's some real test failures in this now: |
|
Ugh sorry about that, much flailing going on. Trying to code in an unfamiliar language when too sleepy! |
|
It's cool! We do it to sometimes. Why we added the checks! |
|
|
Hey, not sure how it happened since we haven't merged in a few days, but your branch got out of date with master. |
|
LGTM. Please hit the "update branch" button to so that we can merge. Thanks again! |
The RFC 5321 algorithm is to check the MX records first, and if they are missing, check for address records. At the moment we only check A records since there is no IPv6 support. Fixes #1197
Validate mail domains in the DNS correctly
|
Thanks, @fanf2. We just pushed this to master. |
|
I mean, to production. Woof. |
The RFC 5321 algorithm is to check the MX records first, and
if they are missing, check for address records. At the moment
we only check A records since there is no IPv6 support.
Fixes #1197