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

Server-side key refresh and update registered public keys from origin server #1748

Merged
merged 8 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions client/handle_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -2035,7 +2035,7 @@ Loop:
trailer := resp.HTTPResponse.Trailer
if errorStatus := trailer.Get("X-Transfer-Status"); errorStatus != "" {
statusCode, statusText := parseTransferStatus(errorStatus)
if statusCode != 200 {
if statusCode != http.StatusOK {
log.WithFields(fields).Debugln("Got error from file transfer")
err = errors.New("transfer error: " + statusText)
return
Expand All @@ -2044,7 +2044,7 @@ Loop:
}
// Valid responses include 200 and 206. The latter occurs if the download was resumed after a
// prior attempt.
if resp.HTTPResponse.StatusCode != 200 && resp.HTTPResponse.StatusCode != 206 {
if resp.HTTPResponse.StatusCode != http.StatusOK && resp.HTTPResponse.StatusCode != http.StatusPartialContent {
log.WithFields(fields).Debugln("Got failure status code:", resp.HTTPResponse.StatusCode)
return 0, 0, -1, serverVersion, &HttpErrResp{resp.HTTPResponse.StatusCode, fmt.Sprintf("Request failed (HTTP status %d): %s",
resp.HTTPResponse.StatusCode, resp.Err().Error())}
Expand Down Expand Up @@ -2271,7 +2271,9 @@ Loop:
log.Debugln("File closed")
case response := <-responseChan:
attempt.ServerVersion = response.Header.Get("Server")
if response.StatusCode != 200 {
// Note: Accept both 200 and 201 as success codes; the latter is the correct one, but
// older versions of XRootD incorrectly use 200.
if response.StatusCode != http.StatusOK && response.StatusCode != http.StatusCreated {
log.Errorln("Got failure status code:", response.StatusCode)
lastError = &HttpErrResp{response.StatusCode, fmt.Sprintf("Request failed (HTTP status %d)",
response.StatusCode)}
Expand Down Expand Up @@ -2337,7 +2339,10 @@ func runPut(request *http.Request, responseChan chan<- *http.Response, errorChan
}
dump, _ = httputil.DumpResponse(response, true)
log.Debugf("Dumping response: %s", dump)
if response.StatusCode != 200 {
// Note: XRootD used to always return 200 (OK) on upload, even when it was supposed to turn
// HTTP 201 (Created). Check for both here; in the future we may want to remove the 200 check
// if we decide to drop support for the older versions of XRootD.
if response.StatusCode != http.StatusOK && response.StatusCode != http.StatusCreated {
log.Errorln("Error status code:", response.Status)
log.Debugln("From the server:")
textResponse, err := io.ReadAll(response.Body)
Expand Down
38 changes: 31 additions & 7 deletions cmd/generate_keygen.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,40 @@
package main

import (
"crypto"
"crypto/elliptic"
"encoding/json"
"fmt"
"os"
"path/filepath"
"strings"

"github.com/lestrrat-go/jwx/v2/jwk"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/viper"

"github.com/pelicanplatform/pelican/config"
"github.com/pelicanplatform/pelican/param"
)

func createJWKS(privKey crypto.PrivateKey) (jwk.Set, error) {
key, err := jwk.FromRaw(privKey)
if err != nil {
return nil, errors.Wrap(err, "failed to generate JWK from private key")
}
jwks := jwk.NewSet()

pkey, err := jwk.PublicKeyOf(key)
if err != nil {
return nil, errors.Wrapf(err, "failed to generate public key from key %s", key.KeyID())
}

if err = jwks.AddKey(pkey); err != nil {
return nil, errors.Wrapf(err, "Failed to add public key %s to new JWKS", key.KeyID())
}

return jwks, nil
}

func keygenMain(cmd *cobra.Command, args []string) error {
wd, err := os.Getwd()
if err != nil {
Expand Down Expand Up @@ -68,15 +88,19 @@ func keygenMain(cmd *cobra.Command, args []string) error {
return fmt.Errorf("file exists for public key under %s", publicKeyPath)
}

viper.Set(param.IssuerKey.GetName(), privateKeyPath)
if err := config.GeneratePrivateKey(privateKeyPath, elliptic.P256(), false); err != nil {
return errors.Wrapf(err, "failed to generate new private key at %s", privateKeyPath)
}
privKey, err := config.LoadPrivateKey(privateKeyPath, false)
if err != nil {
return errors.Wrapf(err, "failed to load private key from %s", privateKeyPath)
}

// GetIssuerPublicJWKS will generate the private key at IssuerKey if it does not exist
// and parse the private key and generate the corresponding public key for us
pubkey, err := config.GetIssuerPublicJWKS()
pubJWKS, err := createJWKS(privKey)
if err != nil {
return err
}
bytes, err := json.MarshalIndent(pubkey, "", " ")
bytes, err := json.MarshalIndent(pubJWKS, "", " ")
if err != nil {
return errors.Wrap(err, "failed to generate json from jwks")
}
Expand Down
6 changes: 2 additions & 4 deletions cmd/generate_keygen_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import (

// Create tmpdir, change cwd, and setup clean up functions
func setupTestRun(t *testing.T) string {
config.ResetIssuerJWKPtr()
config.ResetIssuerPrivateKeys()
wd, err := os.Getwd()
require.NoError(t, err)

Expand All @@ -45,7 +45,6 @@ func setupTestRun(t *testing.T) string {
err := os.Chdir(wd)
require.NoError(t, err)
server_utils.ResetTestState()
config.ResetIssuerJWKPtr()
})
return tmpDir
}
Expand All @@ -64,11 +63,10 @@ func checkKeys(t *testing.T, privateKey, publicKey string) {
}

func TestKeygenMain(t *testing.T) {
config.ResetIssuerJWKPtr()
config.ResetIssuerPrivateKeys()

t.Cleanup(func() {
server_utils.ResetTestState()
config.ResetIssuerJWKPtr()
})

t.Run("no-args-gen-to-wd", func(t *testing.T) {
Expand Down
9 changes: 1 addition & 8 deletions cmd/registry_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,12 @@ import (
"net/url"
"os"

"github.com/lestrrat-go/jwx/v2/jwk"
"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/spf13/viper"

"github.com/pelicanplatform/pelican/config"
"github.com/pelicanplatform/pelican/param"
"github.com/pelicanplatform/pelican/registry"
)

Expand Down Expand Up @@ -109,16 +107,11 @@ func registerANamespace(cmd *cobra.Command, args []string) {
os.Exit(1)
}

privateKeyRaw, err := config.LoadPrivateKey(param.IssuerKey.GetString(), false)
privateKey, err := config.GetIssuerPrivateJWK()
if err != nil {
log.Error("Failed to load private key", err)
os.Exit(1)
}
privateKey, err := jwk.FromRaw(privateKeyRaw)
if err != nil {
log.Error("Failed to create JWK private key", err)
os.Exit(1)
}

if withIdentity {
// We haven't added support to pass sitename from CLI, so leave it empty
Expand Down
12 changes: 8 additions & 4 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,7 @@ func SetServerDefaults(v *viper.Viper) error {
v.SetDefault(param.Xrootd_Authfile.GetName(), filepath.Join(configDir, "xrootd", "authfile"))
v.SetDefault(param.Xrootd_MacaroonsKeyFile.GetName(), filepath.Join(configDir, "macaroons-secret"))
v.SetDefault(param.IssuerKey.GetName(), filepath.Join(configDir, "issuer.jwk"))
v.SetDefault(param.IssuerKeysDirectory.GetName(), filepath.Join(configDir, "issuer-keys"))
v.SetDefault(param.Server_UIPasswordFile.GetName(), filepath.Join(configDir, "server-web-passwd"))
v.SetDefault(param.Server_UIActivationCodeFile.GetName(), filepath.Join(configDir, "server-web-activation-code"))
v.SetDefault(param.OIDC_ClientIDFile.GetName(), filepath.Join(configDir, "oidc-client-id"))
Expand Down Expand Up @@ -1409,12 +1410,12 @@ func InitServer(ctx context.Context, currentServers server_structs.ServerType) e
return err
}

// Reset issuerPrivateJWK to ensure test cases can use their own temp IssuerKey
ResetIssuerJWKPtr()
// Reset IssuerKeys to ensure test cases can use their own temp IssuerKeysDirectory
ResetIssuerPrivateKeys()

// As necessary, generate private keys, JWKS and corresponding certs

// Note: This function will generate a private key in the location stored by the viper var "IssuerKey"
// Note: This function will generate a private key in the location stored by the viper var "IssuerKeysDirectory"
// iff there isn't any valid private key present in that location
_, err = GetIssuerPublicJWKS()
if err != nil {
Expand Down Expand Up @@ -1465,6 +1466,8 @@ func SetClientDefaults(v *viper.Viper) error {
configDir := v.GetString("ConfigDir")

v.SetDefault(param.IssuerKey.GetName(), filepath.Join(configDir, "issuer.jwk"))
v.SetDefault(param.IssuerKeysDirectory.GetName(), filepath.Join(configDir, "issuer-keys"))

upperPrefix := GetPreferredPrefix()
if upperPrefix == OsdfPrefix || upperPrefix == StashPrefix {
v.SetDefault("Federation.TopologyNamespaceURL", "https://topology.opensciencegrid.org/osdf/namespaces")
Expand Down Expand Up @@ -1618,7 +1621,8 @@ func ResetConfig() {
globalFedInfo = pelican_url.FederationDiscovery{}
globalFedErr = nil

ResetIssuerJWKPtr()
ResetIssuerPrivateKeys()

ResetClientInitialized()

// other than what's above, resetting Origin exports will be done by ResetTestState() in server_utils pkg
Expand Down
14 changes: 6 additions & 8 deletions config/encrypted.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"crypto/aes"
"crypto/cipher"
"crypto/ed25519"
"crypto/elliptic"
"crypto/rand"
"crypto/sha256"
"crypto/sha512"
Expand All @@ -42,8 +41,6 @@ import (
"golang.org/x/crypto/nacl/box"
"golang.org/x/term"
"gopkg.in/yaml.v3"

"github.com/pelicanplatform/pelican/param"
)

// If we prompted the user for a new password while setting up the file,
Expand Down Expand Up @@ -380,17 +377,18 @@ func SaveConfigContents_internal(config *OSDFConfig, forcePassword bool) error {
// Take a hash, and use the hash's bytes as the secret.
func GetSecret() (string, error) {
// Use issuer private key as the source to generate the secret
issuerKeyFile := param.IssuerKey.GetString()
err := GeneratePrivateKey(issuerKeyFile, elliptic.P256(), false)
privateKey, err := GetIssuerPrivateJWK()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm - this is going to be a problem.

The secret is used to encrypt the private parts of the config then again to decrypt when reading. This means that we can't rotate out the key.

Any thoughts on how we could fix this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought after some reflection --

The encrypted config is used almost exclusively on the client side for now. Thus, this becomes a blocker for removing old keys but not adding new keys. If the config module had an API to get the oldest key in addition to the current issuer, the issue is avoided until we want to tackle automatic key rotation.

So, one way forward:

  • Add this new API to config to get the "oldest" signing key.
  • Update this call to use the new API.
  • File an issue to introduce a stable secret key to be used for encrypting client configuration. This way, we don't have to do the full solution now.

if err != nil {
return "", err
}
privateKey, err := LoadPrivateKey(issuerKeyFile, false)
if err != nil {

// Extract the underlying ECDSA private key in native Go crypto key type
var rawKey interface{}
if err := privateKey.Raw(&rawKey); err != nil {
return "", err
}

derPrivateKey, err := x509.MarshalPKCS8PrivateKey(privateKey)
derPrivateKey, err := x509.MarshalPKCS8PrivateKey(rawKey)

if err != nil {
return "", err
Expand Down
21 changes: 11 additions & 10 deletions config/encrypted_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ func TestGetSecret(t *testing.T) {
})
t.Run("generate-32B-hash", func(t *testing.T) {
tmp := t.TempDir()
keyName := filepath.Join(tmp, "issuer.key")
viper.Set(param.IssuerKey.GetName(), keyName)
keyDir := filepath.Join(tmp, "issuer-keys")
viper.Set(param.IssuerKeysDirectory.GetName(), keyDir)

get, err := GetSecret()
require.NoError(t, err)
Expand All @@ -55,8 +55,8 @@ func TestEncryptString(t *testing.T) {

t.Run("encrypt-without-err", func(t *testing.T) {
tmp := t.TempDir()
keyName := filepath.Join(tmp, "issuer.key")
viper.Set(param.IssuerKey.GetName(), keyName)
keyDir := filepath.Join(tmp, "issuer-keys")
viper.Set(param.IssuerKeysDirectory.GetName(), keyDir)

get, err := EncryptString("Some secret to encrypt")
require.NoError(t, err)
Expand All @@ -72,8 +72,8 @@ func TestDecryptString(t *testing.T) {
})
t.Run("decrypt-without-err", func(t *testing.T) {
tmp := t.TempDir()
keyName := filepath.Join(tmp, "issuer.key")
viper.Set(param.IssuerKey.GetName(), keyName)
keyDir := filepath.Join(tmp, "issuer-keys")
viper.Set(param.IssuerKeysDirectory.GetName(), keyDir)

secret := "Some secret to encrypt"

Expand All @@ -88,17 +88,18 @@ func TestDecryptString(t *testing.T) {

t.Run("diff-secrets-yield-diff-result", func(t *testing.T) {
tmp := t.TempDir()
keyName := filepath.Join(tmp, "issuer.key")
viper.Set(param.IssuerKey.GetName(), keyName)
keyDir := filepath.Join(tmp, "issuer-keys")
viper.Set(param.IssuerKeysDirectory.GetName(), keyDir)

secret := "Some secret to encrypt"

getEncrypt, err := EncryptString(secret)
require.NoError(t, err)
assert.NotEmpty(t, getEncrypt)

newKeyName := filepath.Join(tmp, "new-issuer.key")
viper.Set(param.IssuerKey.GetName(), newKeyName)
ResetConfig()
newKeyDir := filepath.Join(tmp, "new-issuer-keys")
viper.Set(param.IssuerKeysDirectory.GetName(), newKeyDir)

getDecrypt, err := DecryptString(getEncrypt)
require.NoError(t, err)
Expand Down
Loading
Loading