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

fix(app-builder-lib): signing certificate selection by sha1 #4499

Merged

Conversation

torywheelwright
Copy link
Contributor

Resolves #4291

@torywheelwright
Copy link
Contributor Author

What can I do to help this PR get merged?

@develar develar merged commit 1f2865b into electron-userland:master Jan 16, 2020
@Feverqwe
Copy link

Feverqwe commented Jan 28, 2020

Now it return true when certificateSha1 is null and set correct certificateSubjectName. I think it works incorrect.

(('b' != null && !'abc'.includes('b')) || 'some hash' !== null) // true

From here:
1f2865b#commitcomment-36999255

@Feverqwe
Copy link

Maybe should be something like that:

if (certificateSubjectName !== null && !certInfo.Subject.includes(certificateSubjectName)) {
  continue;
}

if (certificateSha1 !== null && certInfo.Thumbprint !== certificateSha1) {
  continue;
}

@kzimny
Copy link

kzimny commented Feb 2, 2020

Look at #4631 The output of powershell shows the thumprint is uppercase and you compare to original thumprint in lowercase.
The fix:

if ((certificateSubjectName != null && !certInfo.Subject.includes(certificateSubjectName))
    || certInfo.Thumbprint.toLowerCase() !== certificateSha1)

@pablocar80
Copy link

@kzimny that would make the code not work when the user types the thumbprint in uppercase. This can happen if they copy and paste from powershell.

Given that powershell returns already in uppercase, the code can convert what the user typed to uppercase and then compare.

Also, if the user did not specify any sha1, then any certificate that matches the subject should be allowed, according to documentation.

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.

certificateSha1 configuration property isn't respected
5 participants