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

[WIP] Warn in cert-request if CSR doesn't contain SAN #773

Closed
wants to merge 2 commits into from

Conversation

felipevolpone
Copy link
Member

@felipevolpone felipevolpone commented May 9, 2017

The code is not "production-ready", however, I would like to know if I'm on the right path.

AFAIK we should check if the SAN extension is provided and if it has DNSName info.

Fix: https://pagure.io/freeipa/issue/6663

@frasertweedale
Copy link
Contributor

Was there agreement that this should be implemented? (I am personally
against it, because the next release should update the default profile to use
the new CommonNameToSanExtDefault profile component).

If we do implement this, IMO it should be a per-profile configuration, because there may
be legitimate use cases where SAN is not needed.

If we do pursue the current approach, we should further check not only that SAN
is present, but that it contains a DNSName. Put another way, with the current patch,
SAN can be present, but it might contain only KRB5PrincipalName and no DNSName,
and therefore the warning will not show, but it probably should have warned.

@HonzaCholasta
Copy link
Contributor

@frasertweedale, I'm not aware of any agreement and I'm against this as well.

has_dns_in_san_ext = True

if not ext_san or not has_dns_in_san_ext:
print('Warning: The SAN extension '
Copy link
Contributor

@stlaz stlaz May 10, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, I assume you want show warning to user not just in httpd log. For this case you have to use self.add_message() method in plugin, otherwise warning is not propagated to client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you were trying to show the message to the user, do that ^ instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used print to make clear that the code was not in its final version and that I was trying to show the message to the user; I just didn't know how to do it. I'll try to be more straightforward next time. ty!

@pvoborni
Copy link
Member

AFAIK, there was not an agreement not implementing this, otherwise the ticket would be closed. The ticket #6663 was created to warn until the change in profiles is implemented(#4970). It was mentioned yesterday on IPA meeting that we want to warn - when discussing: https://bugzilla.redhat.com/show_bug.cgi?id=1445345 and https://bugzilla.redhat.com/show_bug.cgi?id=1445927

@pvoborni
Copy link
Member

I don't think it makes sense to spend time on configuration of warning - that is larger change (ldap attr, schema, api...) and as such out of scope of 4.5.

Simple warning is IMO good, but it should be worded in a sense that SAN is not always needed. Probably mention in what general use cases it is needed e.g. web services/pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants