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

Using the mackms package to load keys that do not have a tag attribute set #595

Closed
jwright-stripe opened this issue Sep 18, 2024 · 5 comments · Fixed by #600
Closed

Using the mackms package to load keys that do not have a tag attribute set #595

jwright-stripe opened this issue Sep 18, 2024 · 5 comments · Fixed by #600
Assignees
Labels
enhancement New feature or request needs triage

Comments

@jwright-stripe
Copy link

jwright-stripe commented Sep 18, 2024

What would you like to be added?

Hey there!

I really like the mackms package- it makes working with the keychain from Go really pleasant. 🎉

One issue I'm running into is that the current URI structure sets a default tag attribute if one is not provided. This makes it not possible to work with existing keys that do not have a tag on them today.

It doesn't appear possible to explicitly tell mackms to not add a tag attribute to the SecItemCopyMatching queries.

Is this something that you'd be open to considering? I know that changing the existing default behavior wouldn't be backwards compatible. Just tossing out an option, maybe an approach for supporting null* style parameters in the URI, e.g. label=my-name;hash=ccb792f9d9a1262bfb814a339876f825bdba1261;nulltag=true might work for explicitly unsetting attributes.

Note that this might require adding more conditionals like this one so that tags are truly excluded from the search dictionary.

Why this is needed

This will make it easier to work with existing keychain items that may not have particular attributes like tags.

I'm happy to help with this if there's an approach you like, but either way I appreciate y'all considering this! Keep up the great work.

@dopey
Copy link
Contributor

dopey commented Sep 25, 2024

Hey @jwright-stripe 👋, thanks for opening the issue!

A couple questions before addressing the specific issue -

  1. Are you using this project as a dependency of https://smallstep/certificates or just pulling it in on it's own?
  2. If you're using this project downstream of step-ca, we'd love to understand more about the use case in case it's something we should consider featuring in step-ca (or potentially productizing in our commercial products).

On to the issue, we discussed it briefly during a triage meeting earlier today and it sounded like it wouldn't be too challenging to update how the tagging works.

I think @maraino had an idea of how to implement, so I'll let him take it from here.

Cheers 🍻

@jwright-stripe
Copy link
Author

Hey @dopey!

Great questions.

I'm just pulling this in on its own. We have some tooling that manages keys in the macOS keychain today and I happened to notice the mackms library that does the same thing in about as elegant pattern as you can get with macOS bindings. So I was hoping to replace some of the internal implementations with this library when I came across the populated tag requirement.

That said, I'm a big fan of step-ca and pretty much y'all's entire ecosystem so while we don't leverage it today, just know I'm cheering for y'all on the sidelines- y'all are doing great work across the board! 🎉

And that sounds good re: the implementation. If there's an approach y'all think fits the design goals you have, let me know and I'd be happy to take a pass at implementation to save y'all some time.

@maraino
Copy link
Contributor

maraino commented Sep 26, 2024

@jwright-stripe I've created the PR #600 that implements this. But instead of using the parameter nulltag=true, it simply uses the empty tag, tag= (or just mackms:label=test;tag)

Feel free to add comments to the PR, if you think the approach is not the right one.

@jwright-stripe
Copy link
Author

@maraino That approach sounds great to me! Very clever, and setting an empty tag value in the URI is perfectly reasonable to us.

Thank you so much!

@maraino
Copy link
Contributor

maraino commented Sep 26, 2024

@jwright-stripe I've just tagged v0.53.0 with the new changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants