-
Notifications
You must be signed in to change notification settings - Fork 345
Bagajjal/fix minor issues #568
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
Bagajjal/fix minor issues #568
Conversation
bagajjal
commented
Feb 18, 2022
- Remove unnecessary references to ssh-pkcs11.c
- fix warnings
- Create the duplicate token used for create process(ssh-pkcs11-helper.exe)
- Delete and overwrite the ssh-pkcs11 keys when ssh-add.exe is rerun again.
@bagajjal , I think you might have missed my comment here. Failing to add a provider if it already exists is done intentionally as this is the upstream behavior of openssh/openssh-portable. It also makes sense for the user to explicitly remove a provider that already exists before re-adding. |
@@ -37,7 +38,7 @@ | |||
#define BUFSIZE 5 * 1024 | |||
|
|||
char* sshagent_con_username; | |||
int sshagent_client_pid; | |||
HANDLE sshagent_client_primary_token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bagajjal , Exposing the token itself as a global variable is a bit riskier than exposing the client's pid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bagajjal , Exposing the token itself as a global variable is a bit riskier than exposing the client's pid.
We already do this in multiple places. I don't see why it's riskier.
extern HANDLE password_auth_token; |
It was an intentional change. To me it doesn't make sense to fail it. If we fail, the error message is not clear as to why it's failing. I don't see failure message in the upstream code. |