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 explanation on certificate identity retrieval for macOS and Windows #1473

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

wiktor-k
Copy link
Contributor

@wiktor-k wiktor-k commented Oct 2, 2023

Adds a section about certificate's identity on macOS. It took me a while to figure it out and the section was not as comprehensive as its Windows counterpart.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't argue that this is the command that you need to invoke to retrieve the certificate identity. What I'm not clear on is why it's needed. Why should this be in the Briefcase docs? What's the use case that leads to this being required? Briefcase should be managing certificate usage, extracting identities etc.

In terms of the specifics of the implementation - this is currently failing CI because it's missing a changenote.

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Oct 3, 2023

Hi,

What I'm not clear on is why it's needed. Why should this be in the Briefcase docs?

The exact same section is in Windows docs: https://briefcase.readthedocs.io/en/latest/how-to/code-signing/windows.html#certificate-s-sha-1-thumbprint so I thought it would be good too make it consistent and add the same section in macOS: https://briefcase.readthedocs.io/en/latest/how-to/code-signing/macOS.html

What's the use case that leads to this being required? Briefcase should be managing certificate usage, extracting identities etc.

Maybe I'm holding it wrong but briefcase package can be ran in two modes: --adhoc-sign where it uses developer's certificate (and is not for distribution) or --identity IDENTITY where you need the identity of code signing key.

I've been following briefcase docs on Windows and they provided step-by-step instructions on how to get the IDENTITY. When I did the same on macOS the "how to get IDENTITY" section was missing. I spent some time to figure out the command, tested that it works, and thought I'll improve your documentation for the next person that stumbles on the same roadblock as me.

In terms of the specifics of the implementation - this is currently failing CI because it's missing a changenote.

Thanks for the explanation. I'll fix it right away!

@wiktor-k wiktor-k force-pushed the add-macos-identity branch 2 times, most recently from 2862a2c to 46a2065 Compare October 3, 2023 06:42
@freakboy3742
Copy link
Member

What I'm not clear on is why it's needed. Why should this be in the Briefcase docs?

The exact same section is in Windows docs: https://briefcase.readthedocs.io/en/latest/how-to/code-signing/windows.html#certificate-s-sha-1-thumbprint so I thought it would be good too make it consistent and add the same section in macOS: https://briefcase.readthedocs.io/en/latest/how-to/code-signing/macOS.html

Sure - consistency is a reasonable argument. However, there's one other detail to consider:

What's the use case that leads to this being required? Briefcase should be managing certificate usage, extracting identities etc.

Maybe I'm holding it wrong but briefcase package can be ran in two modes: --adhoc-sign where it uses developer's certificate (and is not for distribution) or --identity IDENTITY where you need the identity of code signing key.

You can specify --identity at the command line - or, if you omit that option, you're presented with a list of options to select. When you select an identity from this list, the menu then gives you the "shortcut" version you can use:

(venv3.11) rkm@eunectes togatest % briefcase package

Select code signing identity to use:

  1) Ad-hoc identity. The resulting package will run but cannot be re-distributed.
  2) Apple Development: Russell Keith-Magee (AAAAAA)
  3) Developer ID Application: Russell Keith-Magee (XXXXXX)

> 3

In the future, you could specify this signing identity by running:

    $ briefcase package macOS -i AAAAAAAAABBBBBBBBCCCCCCCCCDDDDDDDD

or

    $ briefcase package macOS -i "Developer ID Application: Russell Keith-Magee (XXXXX)"

[togatest] Signing app with identity Developer ID Application: Russell Keith-Magee (XXXXXX)...
...

So - you don't need to have native options for getting signing identities, because Briefcase will extract them for you.

Unfortunately, this option isn't available on Windows, because there isn't a convenient way (that we've been able to find) to provide a list of all available identities. Therefore, the Windows instructions need to provide a way to get the identities that are valid. This is something that would be nice to fix in the long term, but for now, it's what we've got.

I've been following briefcase docs on Windows and they provided step-by-step instructions on how to get the IDENTITY. When I did the same on macOS the "how to get IDENTITY" section was missing. I spent some time to figure out the command, tested that it works, and thought I'll improve your documentation for the next person that stumbles on the same roadblock as me.

That's definitely appreciated as a goal; I just want to make sure we're fixing the right thing. In this case, it seems like pointing out that specifying an identity is technically optional is the way we should be heading.

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Oct 3, 2023

That's definitely appreciated as a goal; I just want to make sure we're fixing the right thing.

I was looking for an non-interactive way to run briefcase package but if this change is not in scope I guess closing the PR is the right thing to do.

In this case, it seems like pointing out that specifying an identity is technically optional is the way we should be heading.

Understood.

@wiktor-k wiktor-k closed this Oct 3, 2023
@freakboy3742
Copy link
Member

That's definitely appreciated as a goal; I just want to make sure we're fixing the right thing.

I was looking for an non-interactive way to run briefcase package but if this change is not in scope I guess closing the PR is the right thing to do.

To be clear - a change is definitely in scope, because non-interactive use is an intended use. If it's not obvious that Briefcase can tell you how to determine the non-interactive command line, then the documentation definitely needs an update. And there might be a place for having the "getting the identity direct from the source" command in the docs - it just needs to be in the context of why you'd need to use that command (which is what this PR was missing)

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Oct 3, 2023

it just needs to be in the context of why you'd need to use that command (which is what this PR was missing)

Let me clarify: are you saying that the PR is OK just needs a little bit more text adjustments? Something like inserting: "If you need unattended packaging on macOS (briefcase package --identity ...) you need to first retrieve the Developer ID Code Signing certificate identity."

I'm all ears for any other potential text suggestions as, clearly, I'm rather an engineer than a wordsmith 😅

@freakboy3742
Copy link
Member

it just needs to be in the context of why you'd need to use that command (which is what this PR was missing)

Let me clarify: are you saying that the PR is OK just needs a little bit more text adjustments?

Yes.

What is missing is why this new section of the docs is required. "Here's the command to get your identities.... but why do you need to know this? How does this new knowledge fit into the overall workflow? Is this a command I'm going to need to run? If so, why and when?

My suggestion would be that the "next steps" piece is where the elaboration is required. It currently just says "now go use briefcase package. That could be elaborated to say:

  • Once you've installed your certificate, you can run briefcase package
  • By default, you'll be prompted to select from your development identities
  • When you do so, you'll be shown the command line for unattended installation
  • Or, you can run to get the identity, and do an unattended install first time.

but in paragraph form :-)

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Oct 4, 2023

🤔 I've been thinking about it for some time and in my case the issue is that I was expecting to have the identity retrieving command because I did the Windows packaging first and it had that.

So maybe even adding a paragraph in the Next Steps section that says "briefcase package will ask you about the code-signing certificate you want to use and it will give you the command line for unattended installation" would be sufficient? It never occurred to me to run briefcase package (I assumed it will use --adhoc-signing).

The Windows section could also be expanded to say something along the lines "Unfortunately briefcase package cannot retrieve the code-signing certificate's identity automatically so you will need to get it manually." to inform the user that this is not the ideal UX (I though it's normal 😅 ).

Thanks for iterating with me on this!

@freakboy3742
Copy link
Member

🤔 I've been thinking about it for some time and in my case the issue is that I was expecting to have the identity retrieving command because I did the Windows packaging first and it had that.

So maybe even adding a paragraph in the Next Steps section that says "briefcase package will ask you about the code-signing certificate you want to use and it will give you the command line for unattended installation" would be sufficient? It never occurred to me to run briefcase package (I assumed it will use --adhoc-signing).

The Windows section could also be expanded to say something along the lines "Unfortunately briefcase package cannot retrieve the code-signing certificate's identity automatically so you will need to get it manually." to inform the user that this is not the ideal UX (I though it's normal 😅 ).

Both of those sound like really good improvements.

(Of course, the other possible improvement is fix the underlying problem, and sort out what is preventing Winforms from presenting the macOS-style selection menu... :-)

Thanks for iterating with me on this!

My pleasure - thanks for bringing fresh eyes and ideas!

@wiktor-k wiktor-k reopened this Oct 9, 2023
@wiktor-k wiktor-k force-pushed the add-macos-identity branch from 46a2065 to 010102c Compare October 9, 2023 07:06
@wiktor-k wiktor-k changed the title Add Certificate's identity section to macOS code signing Add explanation on certificate identity retrieval for macOS and Windows Oct 9, 2023
@wiktor-k
Copy link
Contributor Author

wiktor-k commented Oct 9, 2023

Both of those sound like really good improvements.

Great! I've re-opened this PR and removed the old section and added new paragraphs.

(Of course, the other possible improvement is fix the underlying problem, and sort out what is preventing Winforms from presenting the macOS-style selection menu... :-)

Yeah... I was thinking the same. It should be possible 🤔 if I ever find out how I'll be sure to create an issue 👍

My pleasure - thanks for bringing fresh eyes and ideas!

😊 Sorry for the delay!

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a couple of minor language edits, but this is great. Thanks for the contribution!

@freakboy3742 freakboy3742 merged commit 042a9f9 into beeware:main Oct 9, 2023
34 checks passed
@wiktor-k
Copy link
Contributor Author

wiktor-k commented Oct 9, 2023

Thanks for help and walking me through the process! This really makes me appreciate the social side of your project (as I already liked what I saw from the technical angle).

Have a nice day! 👋

@wiktor-k wiktor-k deleted the add-macos-identity branch October 9, 2023 19:22
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.

2 participants