Allow multiple private keys but use the latest one

[h2zh]

Overview

In issue #561, we want to rotate out old private key when the new one is provided. This PR mainly changed the functions chain starting from GetIssuerPrivateJWK(), in order to support multiple private key files in a new directory specified by a new config param IssuerKeysDirectory, along with a new goroutine to monitor the changes there every 5 mins.

New Key

In this PR, admin can create new private key through either of the following two ways:

1. Simply drop the private key into into the new directory as a .pem file (the location where config param IssuerKeysDirectory points to, which by default is /etc/pelican/issuer-keys), or modify any .pem there. Pelican program will pick up the last modified .pem file through a goroutine running every 5 mins, set the private key it contains as the active one, add it to an in-memory map, and update the public key in registry sqlite db.
2. Hit the API endpoint “/api/v1.0/origin_ui/newIssuerKey”. Then the new .pem file will be created. Then it will wait for the same goroutine to be loaded (note: The web ui for this API is not included).

Security

How do we know the origin who updates the namespace's public key is the same origin registered this namespace? We authorize the origin on the registry side: require the public keys carried by the key update HTTP request (origin-->registry) must contain the public this origin previously registered, and check the proof of possession for both origin's current active private key and the previous private key, whose can prove the origin owns this namespace(prefix). The latter proof of possession verification uses a "previous private key's signature" sent by origin and verified by the public key stored in db on registry side.

Design thinking

Existing file hierarchy:

├── whatever-parent-dir/ (by default /etc/pelican)                             
│   ├── issuer.jwk            # Existing key location, specified by config param “IssuerKey”

To ensure backward compatibility, the new directory that stores the private keys (saved as .pem files) is mounted to a new config param IssuerKeysDirectory. User doesn’t need to change the value of IssuerKey, though all functions are not longer using it. The private key IssuerKey refers to will be migrated to the new directory specified by IssuerKeysDirectory. The new file hierarchy looks like this:

├── whatever-parent-dir/ (by default /etc/pelican)                             
│   └── issuer-keys/          # The new directory storing multiple private keys where “IssuerKeysDirectory” refers to
│       ├── pelican_generated_<timestamp_1>_<randomChars>.pem    # If no private key is provided, new .pem file will be created
│       ├── pelican_generated_<timestamp_2>_<randomChars>.pem         
│       ├── ...                 
│       ├── <system admin's fav name>.pem
│       └── migrated_<timestamp_2>_<randomChars>.pem    # previous key if it exists, migrated from the location where “IssuerKey” points to

Algorithm efficiency tradeoff

For a map using atomic.pointer, it requires to copy the entire map before update any key-value pair. In the loadPEMFiles func, because it runs every 5 minutes to update the in-memory key map from the file, which is not frequent, we think the memory use by a map using atomic.pointer is still acceptable.

Test

• No private key file
  --> new key created: in-memory active key == public key of this origin's namespaces in registry db

• One existing private key file, adding a new private key
  a) an existing private key file at config param IssuerKey (usually .../issuer.jwk), adding a new private key manually
  b) an existing private key file at config param IssuerKey (usually .../issuer.jwk), adding a new private key via API
  c) an existing private key file at config param IssuerKeysDirectory (usually .../issuer-keys), adding a new private key manually
  d) an existing private key file at config param IssuerKeysDirectory (usually .../issuer-keys), adding a new private key via API
  --> result of any scenario in this section: in-memory active key: second; public key of this origin's namespaces in registry db: second

• Two existing private key files
  a) never run pelican before (empty registry db)
  --> in-memory active key: second; public key of this origin's namespaces in registry db: second
  b) first key is registered in registry db
  --> in-memory active key: second; public key of this origin's namespaces in registry db: second
  c) second key is registered in registry db
  --> in-memory active key: second; public key of this origin's namespaces in registry db: second
  d) second key is registered in registry db, adding the third key through API
  --> in-memory active key: third; public key of this origin's namespaces in registry db: third

Note: "first", "second" key are keys in chronological order of their private key file modification time.
In-memory active key can be copied at the top-right key icon in origin webUI; db public key of this namespace can be found at the registry.sqlite file, whose directory is specified by config param Registry.DbLocation, or in etc/pelican by default

Future work

At this moment, we use the most recent modified .pem file as the active private key. In the next step, we want to allow multiple active private keys in use. #1818

Eventually, when multiple origins have the same namespace, the admin of these origins can append a common private key to all. Then these origin servers will register the corresponding shared public key in the registry. Currently this operation needs to manually done by OSDF admin.

[jhiemstrawisc]

One other quick comment, now that I think we've settled on an answer -- wherever there's some JSON that gets passed back and forth via API calls, we should stick to camel case:
https://github.com/orgs/PelicanPlatform/discussions/1734#discussioncomment-11621001

For example, the new RegisteredPrefixUpdate struct from registry/registry_pubkey_update.go uses snake case when it defines JSON tags like json:"client_nonce". Unless we're worried about a specific backwards compatibility issue here, these should be defined like json:"clientNonce"

[bbockelm]

This is clearly the deadlock and looks like some bad copy/pasting without understanding what the original code does.

The original was:

	defer func() { require.NoError(t, egrp.Wait()) }()
	defer cancel()

Since defer is executed LIFO, this will invoke cancel() followed by the Wait() (to wait on the canceled functions to clean up).

The revised code is effectively this:

         defer func() {
	        func() { require.NoError(t, egrp.Wait()) }()
	        cancel()
          }

Which waits on the group to stop, then shuts it down, a clear deadlock.

[h2zh]

Thanks for pointing out and fixing this lingering problem! Having said that, this is just an intermediate problem in the tests - the root problem is topology mock server doesn't response in TestRegistryKeyChainingOSDF and test timed out in TestRegistryKeyChaining (see the error logs in the tests result). These two errors were suppressed if the other two tests in client_commands_test.go are commented out. Justin figured out there is a test state leak because commenting out the other tests allowed both TestRegistryKeyChaining and TestRegistryKeyChainingOSDF to pass. I'm still looking for the leak and hoping fixing the problems mentioned in your new PR comments could potentially fix the leak.

[h2zh]

	Test result	Test result
Comment Out ⬇️	TestRegistryKeyChainingOSDF	TestRegistryKeyChaining
TestServeNamespaceRegistry	❌	❌
TestMultiPubKeysRegisteredOnNamespace	❌	❌
Both test funcs above	✔️	✔️

[bbockelm]

There's still a lot to do here. Some minor stylistic / convention comments in the review -- plus a few major requests:

1. Redo the handshake between origin and registry to allow arbitrary replacement of the public keys.
2. Move the key refresh logic into the config module and add unit tests. Make the updates atomic and have things in lexicographical order.
3. Do not deprecate the existing issuer key (at least not in this version!) to allow external folks to adapt to the new setup. Do not delete a key that an admin has provided. Instead, behave as if the issuer key was found within the key directory.

[bbockelm]

How does this work? Before, the variable was a file and now it's a directory. All the code below operates on this as if it was a directory.

On first glance, appears that the tool is broken, no?

[h2zh]

Actually, the private/public generation logic lies in config.GetIssuerPublicJWKS(), which was already updated to incorporate the new keys in IssuerKeysDirectory instead of only one key file at IssuerKey. The pelican generate keygen command works as expected (creating a key in ./issuer-keys).

[bbockelm]

Use the appropriate mkstemp to create a unique file. This is not sufficient.

[h2zh]

Done. I also use the mkstemp logic in another similar function GeneratePEM

[bbockelm]

I think the tests should take a different approach: reset the config state between the calls. Otherwise, it clutters up the runtime code.

[bbockelm]

I'm confused by this.

data is from the client -- why is there backward compatibility code in an API call introduced in 7.12?

[bbockelm]

This seems unnecessary.

The client should prove it possess one of the existing keys.

However, this additionally forces the client to keep the key it used for the challenge in the updated key set. That seems unnecessary. We should simply see if the key used to sign is one of the existing known keys.

[bbockelm]

These loops could be replaced by the LookupKeyID method.

[bbockelm]

I think the design here is still a bit confused. For example, it removes the previous key -- not clear that is matching the intent here.

Let me suggest something cleaner:

1. At the start of handshake, the registry sends all known public keys for the namespace.
2. The client receives the list of public keys and selects a corresponding private key from that list (if one is available) to sign the challenge.
3. The client sends the server the updated set of public keys plus the proof of possession from one of the known keys.

Thus, at the end of the handshake:

• The client has demonstrated it owns a private key from the existing known public keys.
• The client's desired set of keys is synchronized with the registry; the client can remove or add any arbitrary keys in the call.

[bbockelm]

Oh! One final item from the review -- please rebase and squash down the commits to a clean list.

Each commit should be self-contained (compiles, passes tests), covering a single piece of functionality (such as adding the key directory logic; or adding the API to update the keys), and have a detailed commit message. Please refer to https://osg-htc.org/technology/software/git-software-development/#making-good-pull-requests-the-art-of-good-commits for some best practices. Given the scope of the changes, I'd expect 4-5 commits to come out on this branch -- not 60.

I typically like keeping the git history in response to reviewer comments to help the reader understand the evolution of ideas. However, at this point, the history of the PR is so messy that it'd be impossible to glean that knowledge -- more value is in a clean branch.