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.

Sign up for GitHub

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

Jump to bottom

Implement an abstact keystore API #3342

Closed
pmatilai opened this issue Sep 30, 2024 · 4 comments · Fixed by #3407
Closed

Implement an abstact keystore API #3342

pmatilai opened this issue Sep 30, 2024 · 4 comments · Fixed by #3407
Labels
API API related RFE

Comments

@pmatilai
Copy link
Member

The support for rpmdb and fs keystores are currently buried in lib/rpmts.c special case code, for historical reasons.
We should add a proper keystore API that supports import, delete and read operations on different backends of choice, and move the relevant parts of the existing code there.
@pmatilai pmatilai added RFE API API related labels Sep 30, 2024
@pmatilai pmatilai added this to RPM Sep 30, 2024
@github-project-automation github-project-automation bot moved this to Backlog in RPM Sep 30, 2024
@pmatilai pmatilai mentioned this issue Sep 30, 2024
Open
@ffesti
Copy link
Contributor

ffesti commented Sep 30, 2024

Needs to account for #3338, #3339, #3340, #3347

pmatilai added a commit to pmatilai/rpm that referenced this issue Oct 22, 2024
@pmatilai
Split keystore handling to a separate source
f080266 
Except for making the three keystore API functions non-static, there
are no code changes here, it's all just moving stuff around. Thus,
there cannot be any functionality changes either.

The internal API here should not be considered final, this is just
a stepping stone to hide the keystore internals from the rest of
the rpmts layer.

Related: rpm-software-management#3342
@pmatilai pmatilai mentioned this issue Oct 22, 2024
Merged
ffesti pushed a commit that referenced this issue Oct 23, 2024
@pmatilai @ffesti
Split keystore handling to a separate source
364d284 
Except for making the three keystore API functions non-static, there
are no code changes here, it's all just moving stuff around. Thus,
there cannot be any functionality changes either.

The internal API here should not be considered final, this is just
a stepping stone to hide the keystore internals from the rest of
the rpmts layer.

Related: #3342
pmatilai added a commit to pmatilai/rpm that referenced this issue Oct 24, 2024
@pmatilai
Drill pubkeys one level deeper into the keystore APIs
91c5d60 
Pass pubkeys down to the keystore on import, and let the keystore handle
the actual format. For rpmdb keystore this remains the header, so it's a
no-op there, but on the fs keystore we drop the trailing "time or something"
hex garbage from the filename because we no longer have it at hand but
also because it's just not relevant for anything. The key load glob is
relaxed enough that we still pick the old format keys too.

Related: rpm-software-management#3342
@pmatilai pmatilai mentioned this issue Oct 24, 2024
Merged
pmatilai added a commit to pmatilai/rpm that referenced this issue Oct 24, 2024
@pmatilai
Drill pubkeys one level deeper into the keystore APIs
0b33ed8 
Pass pubkeys down to the keystore on import, and let the keystore handle
the actual format. For rpmdb keystore this remains the header, so it's a
no-op there, but on the fs keystore we drop the trailing "time or something"
hex garbage from the filename because we no longer have it at hand but
also because it's just not relevant for anything. The key load glob is
relaxed enough that we still pick the old format keys too.

Related: rpm-software-management#3342
ffesti pushed a commit that referenced this issue Oct 28, 2024
@pmatilai @ffesti
Drill pubkeys one level deeper into the keystore APIs
781bf3b 
Pass pubkeys down to the keystore on import, and let the keystore handle
the actual format. For rpmdb keystore this remains the header, so it's a
no-op there, but on the fs keystore we drop the trailing "time or something"
hex garbage from the filename because we no longer have it at hand but
also because it's just not relevant for anything. The key load glob is
relaxed enough that we still pick the old format keys too.

Related: #3342
@pmatilai
Copy link
Member Author

pmatilai commented Oct 28, 2024 •
edited
Loading

AC proposal (or what I have in mind here anyhow):

  • the keystore is a new abstract class
  • the keystore implements three methods: load, import and delete:
    • default constructor & friends - this is only an interface with no internal state
    • virtual rpmRC keystore::load_keys(rpmtxn txn, rpmKeyring keyring)
    • virtual rpmRC keystore::import_key(rpmtxn txn, rpmPubkey key)
    • virtual rpmRC keystore::delete_key(rpmtxn txn, rpmPubkey key)
  • the
  • existing rpmdb and fs keystores are ported to the this class

Edit: method names lowercased
Edit2: mention constructor
Edit3: added virtual
Edit4: added suffixes and double colon to each call to make it legal c++ blush

@pmatilai
Copy link
Member Author

...which is pretty close to what we already have, but having it in a self-sufficient class instead of integer to hold the type should allow for "rebuild" type operation as well.

It's also super minimal, we'll probably want an iterator for the contents later but as this is an internal-only API it can evolve more freely.

pmatilai added a commit to pmatilai/rpm that referenced this issue Oct 28, 2024
@pmatilai
Make keystore read protected by transaction lock too
07ebc22 
Take read-only transaction lock at the rpmts side, pass it down the
rpmKeystoreLoad() hatch.

Somehow this feels like an overkill but then our keystores haven't
been designed with concurrent access in mind *at all*, so play safe
and just take a read lock while loading they keystore. It also balances
the API: everything just takes a txn item now.

Related: rpm-software-management#3342
@pmatilai pmatilai mentioned this issue Oct 28, 2024
Merged
ffesti pushed a commit that referenced this issue Oct 28, 2024
@pmatilai @ffesti
Make keystore read protected by transaction lock too
79b26f1 
Take read-only transaction lock at the rpmts side, pass it down the
rpmKeystoreLoad() hatch.

Somehow this feels like an overkill but then our keystores haven't
been designed with concurrent access in mind *at all*, so play safe
and just take a read lock while loading they keystore. It also balances
the API: everything just takes a txn item now.

Related: #3342
@pmatilai
Copy link
Member Author

Except that the method names are a disaster 😆 because 2/3 conflict with C++ keywords and "load" is just as generic so you never know. Changing these to

  • load_keys
  • import_key
  • delete_key

pmatilai added a commit to pmatilai/rpm that referenced this issue Oct 28, 2024
@pmatilai
Turn keystore into an interface class
f674884 
Add an abstract rpm::keystore class and port our existing rpmdb and fs
keystores to that. The keystore code as such doesn't really change at
all in here, the bigger change is the way it's initialized because it's
an object instead of just an integer in the rpmts struct.

As a kind of side-effect, we introduce the rpm:: namespace here.

Fixes: rpm-software-management#3342
@pmatilai pmatilai mentioned this issue Oct 28, 2024
Merged
ffesti pushed a commit that referenced this issue Oct 28, 2024
@pmatilai @ffesti
Turn keystore into an interface class
2e5730c 
Add an abstract rpm::keystore class and port our existing rpmdb and fs
keystores to that. The keystore code as such doesn't really change at
all in here, the bigger change is the way it's initialized because it's
an object instead of just an integer in the rpmts struct.

As a kind of side-effect, we introduce the rpm:: namespace here.

Fixes: #3342
@github-project-automation github-project-automation bot moved this from Backlog to Done in RPM Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
API API related RFE
Projects
RPM
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement an abstract keystore class + port fs + rpmdb keystores to it pmatilai/rpm
2 participants
@ffesti @pmatilai