ssh-agent icon indicating copy to clipboard operation
ssh-agent copied to clipboard

add option to log public key

Open camilo-celis opened this issue 3 years ago • 4 comments

Should hopefully be a fix for #100

camilo-celis avatar Jul 21 '22 16:07 camilo-celis

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.

mpdude avatar Sep 01 '22 06:09 mpdude

@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.

camilo-celis avatar Sep 02 '22 06:09 camilo-celis

Does this help?

https://github.com/actions/toolkit/pull/725

mpdude avatar Sep 02 '22 06:09 mpdude

Sure, that is what I was referring to exactly. Specially this: https://github.com/actions/toolkit/pull/725#issuecomment-828120911

... 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. ...

camilo-celis avatar Sep 05 '22 11:09 camilo-celis

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

Thank you @camilo-celis !

mpdude avatar Oct 19 '22 10:10 mpdude