Adds support for smartcard identities #1181

thiagohmcruz · 2021-06-01T15:32:01Z

Resolves #1180

So signed builds are possible when using CryptoTokenKit based solutions to store signing identities (e.g. Signing Manager). When storing identities that way security find-identity won't be able to find them.

The proposal in this PR is to append to the current list of SHA1 the values for the smartcard identities found on the system. Please see detailed explanation in the linked issue #1180 for why system_profiler SPSmartCardsDataType -xml is possibly a good solution here.

I'm also 100% open to changing the approach if you spot any issues with this implementation!

Note that the behaviour triggered by setting the --ios_signing_cert_name flag continues to exist: try to find a signing identity by name and return it's SHA1 or return all SHA1 found if the flag is not provided.

google-cla · 2021-06-01T15:32:06Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-06-01T19:42:50Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-06-03T15:14:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

brentleyjones · 2021-06-03T15:54:16Z

Just an FYI, we can't really review the change until the CLA is signed

thiagohmcruz · 2021-06-03T16:05:59Z

Just an FYI, we can't really review the change until the CLA is signed

Yep! Dealing with this internally, I believe the setup is done I might have to re-open the PR though (not 100% sure I'll ask). Working on making this reviewable as we speak.

tools/codesigningtool/codesigningtool.py

thiagohmcruz · 2021-06-03T16:24:18Z

@googlebot I signed it!

tools/codesigningtool/codesigningtool.py

timsutton · 2021-06-03T16:38:04Z

One thing we probably will want to establish is if this constrains macOS versions that could be supported. Do we care about supporting anything older than Mojave (10.14) or Catalina (10.15), for example? That might help inform the logic in what's being searched for in the system_profiler output.

tools/codesigningtool/codesigningtool.py

thiagohmcruz · 2021-06-03T17:49:37Z

One thing we probably will want to establish is if this constrains macOS versions that could be supported. Do we care about supporting anything older than Mojave (10.14) or Catalina (10.15), for example? That might help inform the logic in what's being searched for in the system_profiler output.

@timsutton wouldn't be enough to fallback to empty array when searching for AVAIL_SMARTCARDS_TOKEN and AVAIL_SMARTCARDS_KEYCHAIN?

Not sure how I would go about checking what is available in each OS version, is there an easy way to do that that I'm not aware of?

timsutton · 2021-06-03T19:36:24Z

@timsutton wouldn't be enough to fallback to empty array when searching for AVAIL_SMARTCARDS_TOKEN and AVAIL_SMARTCARDS_KEYCHAIN?

Yes, that seems like just a good defense anyway. I did just check a 10.14 system and these two keys are there as well (full output here.) The code works there as well, just returning an empty [] as expected on a vanilla system.

Another bit of context I'd forgotten to add in this PR is that persistent tokens (as would be used in Signing Manager or other apps providing a token via an app extension) only showed up in 10.15.4. Prior to then it could have only been possible using physically-insertable smartcards, or that's my understanding anyway.

tools/codesigningtool/codesigningtool.py

thiagohmcruz · 2021-06-04T14:40:44Z

@brentleyjones sorry about the direct ping! Just wanted to check if you know who I should bug to get some reviews?

segiddins

It would be awesome to add some test coverage for the xml parsing being done, because some of it gets a bit complicated, but since there's no coverage for this tool at all yet 😶

segiddins · 2021-06-04T17:46:18Z

tools/codesigningtool/codesigningtool.py

+  ], inputstr=cert, raise_on_failure=True)
+  subject = subject.strip().split('/')
+  cert_cn = [f for f in subject if "CN=" in f][0]
+  cert_cn = cert_cn.replace("CN=", "")


is it possible this could be None ?

I guess we should assume it's possible. I'll add the check instead of accessing [0] here and make it return None. Tested locally and is all good since at the call site the comparison is if identity == common_name:

segiddins · 2021-06-04T17:47:36Z

tools/codesigningtool/codesigningtool.py

+  if len(tokens) == 0:
+    return []
+
+  tokens = tokens[0].get("_items", None)


is only checking the first match an issue?

From the output it's not an issue, all tokens will be located under _items

segiddins · 2021-06-04T18:40:22Z

tools/codesigningtool/codesigningtool.py

+  xml = plistlib.loads(str.encode(xml))
+  if len(xml) == 0:
+    return []
+  xml = xml[0].get("_items", None)


do we need to consider all entries in here?

This is the top level _items that contains all the info we need. I think it's safe to ignore the rest for the purpose of this function

thiagohmcruz · 2021-06-04T21:04:19Z

@segiddins should be good to ?

thiagohmcruz · 2021-06-07T18:50:53Z

@brentleyjones, @keith mind taking a look? Let me know if there's anything else you think I should be addressing here 🙏

jerrymarino

A few inline for now, but overall really excited to see smartcard support in Bazel - this has been a long time coming 👍 One alternative to start using this could be to have a different codesigning tool in rules_ios

tools/codesigningtool/codesigningtool.py

thiagohmcruz · 2021-06-08T18:22:50Z

A few inline for now, but overall really excited to see smartcard support in Bazel - this has been a long time coming 👍 One alternative to start using this could be to have a different codesigning tool in rules_ios

@jerrymarino will work on bumping rules_ios next, we might be able to use it as is. Just needing to rebase you rules_swift fork I think. Still not sure what changes will be necessary but will look into it next.

tools/codesigningtool/codesigningtool.py

brentleyjones · 2021-06-08T18:38:35Z

Thanks @thiagohmcruz!

thiagohmcruz · 2021-06-08T18:43:31Z

Thx you all for the feedback here!

jerrymarino · 2021-06-08T18:48:17Z

@thiagohmcruz ok that sounds good to me. If we're blocked on the rules_swift PRs I'll ready a PR to get rid of the fork.

google-cla bot added the cla: no label Jun 1, 2021

thiagohmcruz force-pushed the thiago/read-smartcard-identities branch from 25830b6 to aade1ea Compare June 1, 2021 19:42

thiagohmcruz force-pushed the thiago/read-smartcard-identities branch from aade1ea to 6cfb372 Compare June 3, 2021 15:14

timsutton reviewed Jun 3, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Show resolved Hide resolved

google-cla bot added cla: yes and removed cla: no labels Jun 3, 2021

thiagohmcruz marked this pull request as ready for review June 3, 2021 16:24

timsutton reviewed Jun 3, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Outdated Show resolved Hide resolved

timsutton reviewed Jun 3, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Show resolved Hide resolved

timsutton reviewed Jun 3, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Show resolved Hide resolved

timsutton reviewed Jun 3, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Outdated Show resolved Hide resolved

timsutton reviewed Jun 3, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Show resolved Hide resolved

thiagohmcruz commented Jun 3, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Outdated Show resolved Hide resolved

thiagohmcruz force-pushed the thiago/read-smartcard-identities branch from 122d201 to 2b78ab2 Compare June 4, 2021 03:27

segiddins approved these changes Jun 4, 2021

View reviewed changes

thiagohmcruz force-pushed the thiago/read-smartcard-identities branch from 2b78ab2 to 7458aaa Compare June 4, 2021 20:02

jerrymarino reviewed Jun 8, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Outdated Show resolved Hide resolved

tools/codesigningtool/codesigningtool.py Outdated Show resolved Hide resolved

Add support for smartcard identities (bazelbuild#1180)

79798c4

thiagohmcruz force-pushed the thiago/read-smartcard-identities branch from 7458aaa to 79798c4 Compare June 8, 2021 18:21

brentleyjones reviewed Jun 8, 2021

View reviewed changes

tools/codesigningtool/codesigningtool.py Show resolved Hide resolved

brentleyjones approved these changes Jun 8, 2021

View reviewed changes

brentleyjones merged commit fe15e1c into bazelbuild:master Jun 8, 2021

thiagohmcruz mentioned this pull request Jun 8, 2021

Bump rules_apple to pick up smartcard identities support bazel-ios/rules_ios#250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for smartcard identities #1181

Adds support for smartcard identities #1181

thiagohmcruz commented Jun 1, 2021 •

edited

Loading

google-cla bot commented Jun 1, 2021

google-cla bot commented Jun 1, 2021

google-cla bot commented Jun 3, 2021

brentleyjones commented Jun 3, 2021

thiagohmcruz commented Jun 3, 2021

thiagohmcruz commented Jun 3, 2021

timsutton commented Jun 3, 2021

thiagohmcruz commented Jun 3, 2021

timsutton commented Jun 3, 2021 •

edited

Loading

thiagohmcruz commented Jun 4, 2021

segiddins left a comment

segiddins Jun 4, 2021

thiagohmcruz Jun 4, 2021

segiddins Jun 4, 2021

thiagohmcruz Jun 4, 2021

segiddins Jun 4, 2021

thiagohmcruz Jun 4, 2021

thiagohmcruz commented Jun 4, 2021

thiagohmcruz commented Jun 7, 2021

jerrymarino left a comment

thiagohmcruz commented Jun 8, 2021

brentleyjones commented Jun 8, 2021

thiagohmcruz commented Jun 8, 2021

jerrymarino commented Jun 8, 2021

Adds support for smartcard identities #1181

Adds support for smartcard identities #1181

Conversation

thiagohmcruz commented Jun 1, 2021 • edited Loading

google-cla bot commented Jun 1, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Jun 1, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Jun 3, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

brentleyjones commented Jun 3, 2021

thiagohmcruz commented Jun 3, 2021

thiagohmcruz commented Jun 3, 2021

timsutton commented Jun 3, 2021

thiagohmcruz commented Jun 3, 2021

timsutton commented Jun 3, 2021 • edited Loading

thiagohmcruz commented Jun 4, 2021

segiddins left a comment

Choose a reason for hiding this comment

segiddins Jun 4, 2021

Choose a reason for hiding this comment

thiagohmcruz Jun 4, 2021

Choose a reason for hiding this comment

segiddins Jun 4, 2021

Choose a reason for hiding this comment

thiagohmcruz Jun 4, 2021

Choose a reason for hiding this comment

segiddins Jun 4, 2021

Choose a reason for hiding this comment

thiagohmcruz Jun 4, 2021

Choose a reason for hiding this comment

thiagohmcruz commented Jun 4, 2021

thiagohmcruz commented Jun 7, 2021

jerrymarino left a comment

Choose a reason for hiding this comment

thiagohmcruz commented Jun 8, 2021

brentleyjones commented Jun 8, 2021

thiagohmcruz commented Jun 8, 2021

jerrymarino commented Jun 8, 2021

thiagohmcruz commented Jun 1, 2021 •

edited

Loading

timsutton commented Jun 3, 2021 •

edited

Loading