Skip to content

Conversation

@afontenot
Copy link
Contributor

This is an important keyctl operation that can have implications on whether a key is readable and shows up in Search operations.

It may not be immediately be apparent that this is a supported operation when calling KEYCTL_LINK, you have to dig through the docs to find this.

man 7 keyrings:

As previously mentioned, keyrings are a special type of key that
contains links to other keys (which may include other keyrings). Keys may be linked to by multiple keyrings.

This is an important keyctl operation that can have implications on
whether a key is readable and shows up in Search operations.

It may not be immediately be apparent that this is a supported operation
when calling KEYCTL_LINK, you have to dig through the docs to find this.

man 7 keyrings:

> As previously mentioned, keyrings are a special type of key that
contains links to other keys (which may include other keyrings). Keys
may be linked to by multiple keyrings.
@afontenot
Copy link
Contributor Author

This one is a little thrown together, I'm willing to rework it and do it differently (e.g. we could make it possible to convert a KeyRingIdentifier to a KeySerialId more explicitly than KeySerialId::new(KeyRingIdentifier as i32) and that might be sufficient for these use cases with a little adjustment to the docs to make clear how to do this.

I do think there does need to be a supported way to link keyrings.

Also willing to add tests and improve the comments if you can do a cursory review and say what's needed.

@landhb
Copy link
Owner

landhb commented Aug 22, 2024

@afontenot Thanks for the PR!

I think your explicit methods are better than having users convert to a raw ID and using existing Key methods to workaround it.

It looks clean, some tests would be great though.

I'll try to get to these PRs this weekend.

Also fixes a minor doc issue in previous commit
@afontenot
Copy link
Contributor Author

@landhb tests have been added.

Implementing set_perms on KeyRing in the future would be useful, it would also allow testing some of the claims made about e.g. write / linking access (throughout the KeyRing implementation, not specific to this commit).

@landhb
Copy link
Owner

landhb commented Mar 27, 2025

Sorry for the delay. I'll try to review the 3 open PRs and cut a release soon.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.09%. Comparing base (2e81f72) to head (93224f3).
Report is 3 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
- Coverage   78.33%   69.09%   -9.24%     
==========================================
  Files           9        9              
  Lines         420      343      -77     
==========================================
- Hits          329      237      -92     
- Misses         91      106      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@landhb landhb merged commit cb7b7c5 into landhb:main Mar 30, 2025
10 of 11 checks passed
@landhb
Copy link
Owner

landhb commented Mar 30, 2025

Thanks @afontenot !

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.

3 participants