Incorporate review feedback on token create tool

Author: jhiemstrawisc

cmd/origin_token.go

@@ -46,7 +46,7 @@ func parseInputSlice(rawSlice *[]string, claimPrefix string) []string {
 }
 
 // Parse claims to tokenConfig, excluding "sub". `claims` should be in the form of
-// <claim_key>=<claim=value>
+// <claim_key>=<claim_value>
 func parseClaimsToTokenConfig(profile string, claims []string) (*token.TokenConfig, error) {
 	tokenProfile, err := token.ParseProfile(profile)
 	if err != nil {

cmd/token.go

@@ -53,6 +53,8 @@ var (
 		Short: "Create a token",
 		RunE:  createToken,
 		Args:  cobra.ExactArgs(1),
+		Example: "To create a read/write token for /some/namespace/path in OSDF: " +
+			"pelican token create --read --write pelican://osg-htc.org/some/namespace/path",
 	}
 )
 
@@ -65,17 +67,18 @@ func init() {
 		"Does not grant the ability to overwrite/modify existing resources")
 	tokenCreateCmd.Flags().BoolP("modify", "m", false, "Create a token with the ability to modify/delete the specified resource.")
 	tokenCreateCmd.Flags().BoolP("stage", "s", false, "Create a token with the ability to stage the specified resource.")
-	tokenCreateCmd.Flags().StringP("scope-path", "P", "", "Specify the path to use when creating the token's scopes. This should generally be "+
+	tokenCreateCmd.Flags().String("scope-path", "", "Specify the path to use when creating the token's scopes. This should generally be "+
 		"the object path without the namespace prefix.")
 
 	// Additional token fields
 	tokenCreateCmd.Flags().StringP("audience", "a", "", "Specify the tokens audience. If not provided, the equivalent 'any' audience "+
 		"for the selected profile will be used (e.g. 'https://wlcg.cern.ch/jwt/v1/any' for the 'wlcg' profile).")
 	tokenCreateCmd.Flags().IntP("lifetime", "l", 1200, "Set the lifetime of the token in seconds. Default is 1200 (20min).")
-	tokenCreateCmd.Flags().StringP("subject", "u", "", "Set the subject of the token. If not provided, the current user will be used as the default subject.")
+	tokenCreateCmd.Flags().String("subject", "", "Set the subject of the token. If not provided, the current user will be used as the default subject.")
 	tokenCreateCmd.Flags().StringP("issuer", "i", "", "Set the issuer of the token. If not provided, the issuer will be discovered via the Director.")
-	tokenCreateCmd.Flags().StringArrayP("claim", "c", []string{}, "Set claims to be added to the token. Format: <claim_key>=<claim_value>. ")
-	tokenCreateCmd.Flags().StringArrayP("scope", "C", []string{}, "Set non-typical values for the token's 'scope' claim. Format: <scope_key>=<scope_value>. ")
+	tokenCreateCmd.Flags().StringArray("raw-claim", []string{}, "Set claims to be added to the token. Format: <claim_key>=<claim_value>. ")
+	tokenCreateCmd.Flags().StringArray("raw-scope", []string{}, "Set non-typical values for the token's 'scope' claim. Scopes should be space-separated, e.g. "+
+		"'storage.read:/ storage.create:/'.")
 	tokenCreateCmd.Flags().StringP("profile", "p", "wlcg", "Create a token with a specific JWT profile. Accepted values are scitokens2 and wlcg")
 	tokenCreateCmd.Flags().StringP("private-key", "k", "", "Path to the private key used to sign the token. If not provided, Pelican will look for the private key in the default location.")
 }
@@ -101,11 +104,9 @@ func verifyIssuer(issuer string, kidSet map[string]struct{}) (bool, error) {
 		return false, err
 	}
 
-	it := (*remoteJWKS).Keys(context.Background())
-	for it.Next(context.Background()) {
-		key := it.Pair().Value.(jwk.Key)
-		if _, ok := kidSet[key.KeyID()]; ok {
-			log.Debugf("Found matching key ID %s in JWKS from issuer %s", key.KeyID(), issuer)
+	for kid := range kidSet {
+		if _, ok := (*remoteJWKS).LookupKeyID(kid); ok {
+			log.Debugf("Found matching key ID %s in JWKS from issuer %s", kid, issuer)
 			return true, nil
 		}
 	}
@@ -120,7 +121,7 @@ func verifyIssuer(issuer string, kidSet map[string]struct{}) (bool, error) {
 // which issuer aligns with their signing key.
 func getIssuer(directorInfo server_structs.DirectorResponse, kidSet map[string]struct{}) (string, error) {
 	if len(directorInfo.XPelAuthHdr.Issuers) == 0 {
-		return "", errors.New("no issuers found for %s in the Director response")
+		return "", errors.Errorf("no issuers found for %s in the Director response", directorInfo.XPelNsHdr.Namespace)
 	}
 
 	// Comb through the JWKS from each issuer to find which matches the signing key
@@ -180,7 +181,8 @@ func createToken(cmd *cobra.Command, args []string) error {
 	// First arg contains the pelican URL of the resource
 	// Cobra has already checked that we have exactly one arg
 	rawUrl := args[0]
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
 	pUrl, pUrlErr := client.ParseRemoteAsPUrl(ctx, rawUrl)
 	sPath, err := cmd.Flags().GetString("scope-path")
 	if err != nil {

config/init_server_creds.go

@@ -748,18 +748,21 @@ func loadIssuerPrivateKey(issuerKeysDir string) (jwk.Key, error) {
 
 // Helper function to load the issuer/server's public key for other servers
 // to verify the token signed by this server. Only intended to be called internally
-func loadIssuerPublicJWKS(existingJWKS string, issuerKeysDir string, pemPath ...string) (jwk.Set, error) {
+//
+// The `keyPathOverridePEM` parameter is used to override the default locations by
+// providing a PEM file path.
+func loadIssuerPublicJWKS(existingJWKS string, issuerKeysDir string, keyPathOverridePEM ...string) (jwk.Set, error) {
 	jwks := jwk.NewSet()
 	if existingJWKS != "" {
 		var err error
 		jwks, err = jwk.ReadFile(existingJWKS)
 		if err != nil {
 			return nil, errors.Wrap(err, "Failed to read issuer JWKS file")
 		}
-	} else if len(pemPath) > 0 && pemPath[0] != "" {
-		key, err := LoadSinglePEM(pemPath[0])
+	} else if len(keyPathOverridePEM) > 0 && keyPathOverridePEM[0] != "" {
+		key, err := LoadSinglePEM(keyPathOverridePEM[0])
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to load signing key from %s", pemPath[0])
+			return nil, errors.Wrapf(err, "failed to load signing key from %s", keyPathOverridePEM[0])
 		}
 		pkey, err := jwk.PublicKeyOf(key)
 		if err != nil {
@@ -798,12 +801,15 @@ func loadIssuerPublicJWKS(existingJWKS string, issuerKeysDir string, pemPath ...
 }
 
 // Return the private JWK for the server to sign tokens and payloads
-func GetIssuerPrivateJWK(signingKey ...string) (jwk.Key, error) {
+//
+// The `keyPathOverridePEM` parameter is used to override the default locations by
+// providing a PEM file path.
+func GetIssuerPrivateJWK(keyPathOverridePEM ...string) (jwk.Key, error) {
 	// If an optional key is provided, load it and return
-	if len(signingKey) > 0 && signingKey[0] != "" {
-		key, err := LoadSinglePEM(signingKey[0])
+	if len(keyPathOverridePEM) > 0 && keyPathOverridePEM[0] != "" {
+		key, err := LoadSinglePEM(keyPathOverridePEM[0])
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to load signing key from %s", signingKey[0])
+			return nil, errors.Wrapf(err, "failed to load signing key from %s", keyPathOverridePEM[0])
 		}
 		return key, nil
 	}
@@ -837,10 +843,13 @@ func GetIssuerPrivateJWK(signingKey ...string) (jwk.Key, error) {
 // this server to sign JWTs it issues. The public key returned will be exposed publicly
 // for other servers to verify JWTs signed by this server, typically via a well-known URL
 // i.e. "/.well-known/issuer.jwks"
-func GetIssuerPublicJWKS(keyPath ...string) (jwk.Set, error) {
+//
+// The `keyPathOverridePEM` parameter is used to override the default locations by
+// providing a PEM file path.
+func GetIssuerPublicJWKS(keyPathOverridePEM ...string) (jwk.Set, error) {
 	existingJWKS := param.Server_IssuerJwks.GetString()
 	issuerKeysDir := param.IssuerKeysDirectory.GetString()
-	return loadIssuerPublicJWKS(existingJWKS, issuerKeysDir, keyPath...)
+	return loadIssuerPublicJWKS(existingJWKS, issuerKeysDir, keyPathOverridePEM...)
 }
 
 // Check if there is a session secret exists at param.Server_SessionSecretFile and is not empty if there is one.