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

Do not log DM password in ca/kra installation logs #231

Closed
wants to merge 1 commit into from

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Nov 10, 2016

We can merge this after refactoring merges not to mess the rebases.

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

@martbab martbab self-assigned this Nov 14, 2016
@martbab
Copy link
Contributor

martbab commented Nov 14, 2016

I would rather hide the password by default in the spawn_instance method in the same manner as is done for admin_password, see https://git.fedorahosted.org/cgit/freeipa.git/tree/ipaserver/install/dogtaginstance.py?id=f183f70e0183e51d569ada972bd3ec73cad76a30#n166

@stlaz stlaz closed this Nov 14, 2016
@stlaz
Copy link
Contributor Author

stlaz commented Nov 18, 2016

I must have misclicked "close" when viewing this PR on my phone. I believe we may rather add admin and DM passwords to the nolog_list at the point where the disclosed credentials file is created so that we avoid problems like this one in the future.

@stlaz stlaz reopened this Nov 18, 2016
@martbab
Copy link
Contributor

martbab commented Nov 21, 2016

Well that is what I was pointing at, by adding both DM and admin passwords to the parent method's default nolog_list, you are future-proofing the code because all spawn-instance calls will be safe. But maybe I am missing something.

@tkrizek
Copy link
Contributor

tkrizek commented Nov 22, 2016

I agree. We need to re-add self.dm_password to nolog_list, just like it was before I removed it here

There is no reason to change it. I originally removed the line, because I thought I could remove dm_password from DogtagInstance all together, but that turned out not to be the case.

@stlaz
Copy link
Contributor Author

stlaz commented Nov 22, 2016

@martbab Oh, I thought you wanted me to re-add dm_password to DogtagInstance as @tomaskrizek which does not seem right as DogtagInstance is in no position to decide what to log and what not as it does not really know what's in that cfg_file it's getting.
Will get it passed from the actual caller of spawn_instance which is either cainstance or krainstance.

@tkrizek
Copy link
Contributor

tkrizek commented Nov 22, 2016

I didn't notice dm_password is no longer in DogtagInstance, I re-added it elsewhere. In that case, as @stlaz said, passing it to spawn_instance() seems like the proper way to do it.

@martbab martbab added the ack Pull Request approved, can be merged label Nov 25, 2016
@martbab
Copy link
Contributor

martbab commented Nov 25, 2016

@martbab martbab added the pushed Pull Request has already been pushed label Nov 25, 2016
@martbab martbab closed this Nov 25, 2016
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
3 participants