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

Jump to bottom

Allow issuing certificates with IP addresses in subjectAltName (ftweedal) #1843

Closed
wants to merge 7 commits into from
Closed

Allow issuing certificates with IP addresses in subjectAltName (ftweedal) #1843

wants to merge 7 commits into from

Conversation

frasertweedale
Copy link
Contributor

Continuation of #1700 by @ipilcher,
adding commits by @ftweedal.

Please keep both PRs open for the time being.

@frasertweedale frasertweedale mentioned this pull request Apr 23, 2018
Closed
ipilcher
ipilcher reviewed Apr 23, 2018
View reviewed changes
Copy link
Contributor

@ipilcher ipilcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CNAME chains are a valid use case in some cases (geographic load balancing, content delivery networks, etc.), so there might be a future benefit to supporting them. If you're going to do that, though, don't you need a mechanism to detect loops?

Also, I think that the debug message is fairly important. The logic of which IPs are considered acceptable is likely to be opaque so some users, so I think it's important that a mechanism exists for them to discover why an IP address was rejected. (I also think that debug messages like this tend to make code easier to understand - almost like quasi-comments.)

@frasertweedale
Copy link
Contributor Author

@ipilcher thanks for your comments.

There is no CNAME loop detection, but there is a maximum depth which for practical purposes is sufficient, IMO.

The debug log cannot be seen by users, so it is of no use to users. I'll restore the debug message anyway. But we'll also need to rework the ValidationError exceptions that get thrown, so that they provide more information.

@frasertweedale frasertweedale added the WIP Work in progress - not ready yet for review label Apr 24, 2018
@ipilcher
Copy link
Contributor

The debug log cannot be seen by users, so it is of no use to users. I'll restore the debug message anyway

Fair point. I should have said "administrators."

But we'll also need to rework the ValidationError exceptions that get thrown, so that they provide more information.

Hmm. The message currently includes the IP address that's being rejected. I'm not sure what else you could reasonably include.

@frasertweedale
Copy link
Contributor Author

@ipilcher I'll push an updated patch next week (hopefully early in the week).

@frasertweedale
Copy link
Contributor Author

@ipilcher sorry, been a bit bogged down in other issues. Will get back to this soon, I hope!

@ipilcher
Copy link
Contributor

ipilcher commented Aug 3, 2018

At this point, would it make sense to just use the "hacky" version in the original PR?

@BongoEADGC6
Copy link

@frasertweedale This would help for users that want to have signed Kubernetes certificates.

@cjeanneret
Copy link

Hello! any news on that one? Would be really useful in many cases.... Thanks!

@frasertweedale
Copy link
Contributor Author

I'm hoping to push this over the line in the next couple of weeks. Thank you all for your patience.

@frasertweedale
Copy link
Contributor Author

Kicking this off again with a rebase. I'll fix whatever needs fixing, add tests, and then we should be good to go.

@frasertweedale
Copy link
Contributor Author

Couple of minor tweaks to come, as well as tests. And a blog post to demo and plug the shiny new feature. Should knock this off next week @ipilcher.

@frasertweedale frasertweedale force-pushed the feature/1700-san-ipaddress branch 2 times, most recently from 8410a6c to c2d563a Compare February 19, 2019 03:48
@frasertweedale
Copy link
Contributor Author

Some tests are added. There are more to write but I want to get some early feedback from a "big" CI run, while I continue expanding the tests for this feature.

@frasertweedale frasertweedale added needs review Pull Request is waiting for a review ipa-next Mark as master (4.12) only and removed WIP Work in progress - not ready yet for review labels Feb 19, 2019
@frasertweedale
Copy link
Contributor Author

Expanded test suite. Labels updated - this is ready for review now!

@frasertweedale frasertweedale added ipa-4-7 ipa-4-6 Mark for backport to ipa 4.6 and removed ipa-next Mark as master (4.12) only labels Feb 19, 2019
@flo-renaud flo-renaud self-assigned this Feb 20, 2019
@flo-renaud
Copy link
Contributor

Hi @frasertweedale
thanks for the PR. So far I have tried (successfully) the following:

  • request a cert for a host with the CSR containing a SAN = host's IP @, IP @ and reverse record in IPA DNS zones
  • request a cert for a host with the CSR containing a SAN = another host's IP @, IP @ and reverse record in IPA DNS zones: properly get an error, but the message is IP Address in SAN does not match any DNS name, it would be more clear with IP Address in SAN does not match the host DNS records.
  • request a cert for a host with the CSR containing a SAN = IP not defined in FreeIPA DNS, error
  • request a cert for a host with the CSR containing a SAN = IP defined in FreeIPA DNS but reverse record missing, error (but the message could be more clear with a hint to reverse DNS record missing).
  • create a kerberos principal alias for the host, add DNS record and reverse record, request a cert for a host with the CSR containing a SAN = IPalias, and DNSalias, success
  • if the DNS record or reverse record for the alias is missing, error as expected
  • if the CSR contains only the IPalias but not the DNS alias, error as expected

I will have a look at the xml tests later, but so far, so good.

@flo-renaud
Copy link
Contributor

The xml tests are good.

My only concern is that a more specific error message would help troubleshoot in case of failure:

  • IP address in SAN does not match any DNS name
  • IP address in SAN does not match any of the hostnames in SAN
  • reverse IP record missing

@ipilcher @frasertweedale
Allow issuing certificates with IP addresses in subjectAltName
df8aa35 
Allow issuing certificates with IP addresses in the subject
alternative name (SAN), if all of the following are true.

* One of the DNS names in the SAN resolves to the IP address
  (possibly through a CNAME).
* All of the DNS entries in the resolution chain are managed by
  this IPA instance.
* The IP address has a (correct) reverse DNS entry that is managed
  by this IPA instance

https://pagure.io/freeipa/issue/7451
@frasertweedale
cert-request: restrict IPAddress SAN to host/service principals
4944fdd 
Part of: https://pagure.io/freeipa/issue/7451
@frasertweedale
cert-request: collect only qualified DNS names for IPAddress validation
a256d72 
Collect only qualified DNS names for IPAddress validation.  This is
necessary because it is undecidable whether the name 'ninja' refers
to 'ninja.my.domain.' or 'ninja.' (assuming both exist).  Remember
that even a TLD can have A records.

Now that we are only checking qualified names for the purpose of
IPAddressName validation, remove the name length hack from
_san_dnsname_ips().

Part of: https://pagure.io/freeipa/issue/7451
@frasertweedale
cert-request: generalise _san_dnsname_ips for arbitrary cname depth
36a9411 
Generalise _san_dnsname_ips to allow arbitrary cname depths.  This
also clarifies the code and avoids boolean blindness.  Update the
call site to maintain the existing behvaiour (one cname allowed).

Part of: https://pagure.io/freeipa/issue/7451
@frasertweedale
Copy link
Contributor Author

@flo-renaud thanks for the review. I pushed a new commit that causes different messages to be raised depending on the error:

  • no forward resolution from DNS names to IP address
  • no PTR record for IP address
  • asymmetric PTR and forward records for IP address

@frasertweedale
cert-request: report all unmatched SAN IP addresses
4023858 
During SAN validation, it is possible that more than one
iPAddressName does not match a known IP address for the DNS names in
the SAN.  But only one unmatched IP address is reported.  Update the
error message to mention all unmatched iPAddressName values.

Part of: https://pagure.io/freeipa/issue/7451
@frasertweedale
Add tests for cert-request IP address SAN support
14c0902 
Part of: https://pagure.io/freeipa/issue/7451
@frasertweedale
cert-request: more specific errors in IP address validation
eb707d5 
Update the IP address validation to raise different error messages
for:

- inability to reach IP address from a DNS name
- missing PTR records for IP address
- asymmetric PTR / forward records

If multiple scenarios apply, indicate the first error (from list
above).

The code should now be a bit easier to follow.  We first build dicts
of forward and reverse DNS relationships, keyed by IP address.  Then
we check that entries for each iPAddressName are present in both
dicts.  Finally we check for PTR-A/AAAA symmetry.

Update the tests to check that raised ValidationErrors indicate the
expected error.

Part of: https://pagure.io/freeipa/issue/7451
@frasertweedale
Copy link
Contributor Author

Removing backport tags - I'll wait for relevant BZs to get ACKed before backporting this feature.

@frasertweedale frasertweedale removed ipa-4-6 Mark for backport to ipa 4.6 ipa-4-7 labels Feb 21, 2019
@flo-renaud
Copy link
Contributor

Hi @frasertweedale
Thanks for the update, the error messages really help. Works for me, ACK.

@flo-renaud flo-renaud added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Feb 21, 2019
@flo-renaud flo-renaud added ipa-4-7 ipa-4-6 Mark for backport to ipa 4.6 labels Mar 4, 2019
@flo-renaud
Copy link
Contributor

master:

  • dccb2e0 Allow issuing certificates with IP addresses in subjectAltName
  • 8ec4868 cert-request: restrict IPAddress SAN to host/service principals
  • eb70e64 cert-request: collect only qualified DNS names for IPAddress validation
  • 9c750f0 cert-request: generalise _san_dnsname_ips for arbitrary cname depth
  • e37c025 cert-request: report all unmatched SAN IP addresses
  • 474a2e6 Add tests for cert-request IP address SAN support
  • a65c12d cert-request: more specific errors in IP address validation

@flo-renaud flo-renaud added the pushed Pull Request has already been pushed label Mar 4, 2019
@flo-renaud flo-renaud closed this Mar 4, 2019
@flo-renaud
Copy link
Contributor

@frasertweedale
A manual backport is required for ipa-4-6 and ipa-4-7 branches, can you handle it? Thanks.

@frasertweedale
Copy link
Contributor Author

@flo-renaud no worries, it's a simple backport but I intend to wait until relevant BZs have been ACKed. Thanks for merging!

This was referenced Mar 7, 2019
Closed
Closed
@frasertweedale frasertweedale deleted the feature/1700-san-ipaddress branch March 7, 2019 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ipilcher ipilcher ipilcher left review comments

Assignees

@flo-renaud flo-renaud

Labels
ack Pull Request approved, can be merged ipa-4-6 Mark for backport to ipa 4.6 pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
5 participants
@frasertweedale @ipilcher @BongoEADGC6 @cjeanneret @flo-renaud