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

add option to log public key #122

Closed
wants to merge 4 commits into from

Conversation

camilo-celis
Copy link
Contributor

@camilo-celis camilo-celis commented Jul 21, 2022

Should hopefully be a fix for #100

@camilo-celis camilo-celis force-pushed the camilo/log-public-key branch 2 times, most recently from 624228f to 212f97d Compare July 21, 2022 16:41
@mpdude
Copy link
Member

mpdude commented Sep 1, 2022

In general: Do you have a good source explaining why public key fingerprints are a security issue? I still don't see the problem with that, and so I'd like to avoid adding any extra code/config option for it, in particular when it takes so many lines of code.

The log messages are intended to help newcoming users in setting up things correctly and to make sure the necessary diagnostic information is there when issues are raised. To not defeat this purpose, at least the feature should be opt-out, not opt-in.

@camilo-celis
Copy link
Contributor Author

@mpdude thanks for your reply. As for your questions and comments:

In general: Do you have a good source explaining why public key fingerprints are a security issue? I still don't see the problem with that, and so I'd like to avoid adding any extra code/config option for it, ...

I'm not implying that but I still think some users of this GHA would find it beneficial if there was an option to do so. And just for completeness, it is not that we are logging out the fingerprint only but the entire public key. We would still log out the added keys as per output of ssh-add -L.

... in particular when it takes so many lines of code.

Sadly, it takes many more lines of code than initially intended because I wanted to force the flag to be a boolean and not required. But there is a shortcoming where the getBooleanInput would not respect the required flag. It is claimed to be by design but I disagree. Anyways, without it, testing is slightly annoying as then the flag would be a required input even if you specify a default on action.yml. I will remove it on a future commit to see if that is better regardless.

The log messages are intended to help newcoming users in setting up things correctly and to make sure the necessary diagnostic information is there when issues are raised. To not defeat this purpose, at least the feature should be opt-out, not opt-in.

Sounds reasonable.

@mpdude
Copy link
Member

mpdude commented Sep 2, 2022

Does this help?

actions/toolkit#725

@camilo-celis
Copy link
Contributor Author

Sure, that is what I was referring to exactly. Specially this: actions/toolkit#725 (comment)

... I think the function is used to deal with boolean input, It should have a default value in action.yml if it is not necessary. And when it should be decided by users, it also should return a type error when users input the wrong value, which will help users to solve it without asking. ...

@mpdude mpdude closed this in fbef2c7 Oct 19, 2022
@mpdude
Copy link
Member

mpdude commented Oct 19, 2022

I've added a few tweaks and squash-merged this through fbef2c7.

Thank you @camilo-celis !

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

Successfully merging this pull request may close these issues.

None yet

2 participants