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

Remove MD5 certificate fingerprints #482

Closed
wants to merge 1 commit into from
Closed

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Feb 20, 2017

To be "backward compatible" we cannot remove md5_fingerprint so we at least supply the reason why it can't be counted.

https://fedorahosted.org/freeipa/ticket/5695

edit: For this to work #437 needs to be pushed first.

@tkrizek tkrizek self-assigned this Feb 20, 2017
@rcritten
Copy link
Contributor

In service.py the error isn't wrapped in _(). You should use the same message in both.

Given the different messages I'm surprised this didn't pop up as a test failure.

@MartinBasti
Copy link
Contributor

MartinBasti commented Feb 20, 2017

I don't think that this is a good way how to handle backward compatibility. With FIPS mode enabled there is no md5 backward compatibility and users should adapt their automation. In case that IPA API is used directly it will contain a garbage and it may not be catched fast enough by any automation on user side. We should not provide anything related to md5 under FIPS mode and let any possible automation using IPA API to fail early on missing values.

@tkrizek
Copy link
Contributor

tkrizek commented Feb 20, 2017

@rcritten Currently, the tests fail because we need #437 merged. It would be caught.

@MartinBasti The only other option I see is to provide None. We can't remove the md5 fingerprint from API - or can we?

@stlaz
Copy link
Contributor Author

stlaz commented Feb 20, 2017

I am fine with not providing md5_fingerprint at all but that would require the tests to be fixed as well and I am not sure how to easily do that in this case.

@tkrizek
Copy link
Contributor

tkrizek commented Feb 20, 2017

Actually, we don't need to provide md5_fingerprint at all in FIPS, since the attribute is marked as vritual_attribute.

MD5 is a grandpa and FIPS does not like it at all.

https://fedorahosted.org/freeipa/ticket/5695
@stlaz stlaz changed the title Don't count service/host/user cert md5 fprints in FIPS Remove MD5 certificate fingerprints Feb 21, 2017
@stlaz
Copy link
Contributor Author

stlaz commented Feb 21, 2017

@rcritten thanks for noticing the discrepancy in the previous version of the commit, it was a leftover from previous implementation.
I reworked the commit to remove MD5 certificate fingerprints completely, leaving just SHA1. Is a requirement to accept this patch to announce this on freeipa-devel/users?

@tkrizek
Copy link
Contributor

tkrizek commented Feb 21, 2017

@stlaz I think it'd be good to discuss this change on freeipa-devel. Also, since we're removing md5, I'd consider adding sha256.

@MartinBasti
Copy link
Contributor

+1 for sha256

@tkrizek
Copy link
Contributor

tkrizek commented Feb 21, 2017

Btw, I think sha256 can be added in a separate PR. Let's just wait if there are any concerns about removing md5 on the freeipa-devel.

@stlaz
Copy link
Contributor Author

stlaz commented Feb 21, 2017 via email

@tkrizek
Copy link
Contributor

tkrizek commented Feb 23, 2017

ACK, there is no disagreement on the freeipa-devel.

I'm already working on replacing SHA1 with SHA256 given the recent events.

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Feb 23, 2017
@MartinBasti
Copy link
Contributor

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Feb 23, 2017
@stlaz stlaz deleted the no_cert_md5 branch September 11, 2017 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants