Resolved PR review comments

core/module/handler/step.go

@@ -108,11 +108,11 @@ func (s *validateSignStep) validate(ctx *model.StepContext, value string) error
 		return fmt.Errorf("malformed sign header")
 	}
 	keyID := headerParts[1]
-	keySet, err := s.km.Keyset(ctx, keyID)
+	signingPublicKey, _, err := s.km.LookupNPKeys(ctx, ctx.SubID, keyID)
 	if err != nil {
 		return fmt.Errorf("failed to get validation key: %w", err)
 	}
-	if err := s.validator.Validate(ctx, ctx.Body, value, keySet.SigningPublic); err != nil {
+	if err := s.validator.Validate(ctx, ctx.Body, value, signingPublicKey); err != nil {
 		return fmt.Errorf("sign validation failed: %w", err)
 	}
 	return nil

pkg/plugin/implementation/keymanager/cmd/plugin.go

@@ -17,8 +17,8 @@ var newKeyManagerFunc = keymanager.New
 // New creates and initializes a new KeyManager instance using the provided cache, registry lookup, and configuration.
 func (k *keyManagerProvider) New(ctx context.Context, cache definition.Cache, registry definition.RegistryLookup, cfg map[string]string) (definition.KeyManager, func() error, error) {
 	config := &keymanager.Config{
-		VaultAddr: cfg["vault_addr"],
-		KVVersion: cfg["kv_version"],
+		VaultAddr: cfg["vaultAddr"],
+		KVVersion: cfg["kvVersion"],
 	}
 	log.Debugf(ctx, "Keymanager config mapped: %+v", cfg)
 	km, cleanup, err := newKeyManagerFunc(ctx, cache, registry, config)

pkg/plugin/implementation/keymanager/cmd/plugin_test.go

@@ -62,8 +62,8 @@ func TestNewSuccess(t *testing.T) {
 	cache := &mockCache{}
 	registry := &mockRegistry{}
 	cfg := map[string]string{
-		"vault_addr": "http://dummy-vault",
-		"kv_version": "2",
+		"vaultAddr": "http://dummy-vault",
+		"kvVersion": "2",
 	}
 
 	cleanupCalled := false
@@ -105,8 +105,8 @@ func TestNewFailure(t *testing.T) {
 	cache := &mockCache{}
 	registry := &mockRegistry{}
 	cfg := map[string]string{
-		"vault_addr": "http://dummy-vault",
-		"kv_version": "2",
+		"vaultAddr": "http://dummy-vault",
+		"kvVersion": "2",
 	}
 
 	newKeyManagerFunc = func(ctx context.Context, cache definition.Cache, registry definition.RegistryLookup, cfg *keymanager.Config) (*keymanager.KeyMgr, func() error, error) {

pkg/plugin/implementation/keymanager/keymanager.go

@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"strings"
 
 	"github.com/beckn/beckn-onix/pkg/log"
 	"github.com/beckn/beckn-onix/pkg/model"
@@ -61,11 +62,13 @@ func ValidateCfg(cfg *Config) error {
 	if cfg.VaultAddr == "" {
 		return errors.New("invalid config: VaultAddr cannot be empty")
 	}
-	if cfg.KVVersion == "" {
-		cfg.KVVersion = "v1"
-	} else if cfg.KVVersion != "v1" && cfg.KVVersion != "v2" {
+	kvVersion := strings.ToLower(cfg.KVVersion)
+	if kvVersion == "" {
+		kvVersion = "v1"
+	} else if kvVersion != "v1" && kvVersion != "v2" {
 		return fmt.Errorf("invalid KVVersion: must be 'v1' or 'v2'")
 	}
+	cfg.KVVersion = kvVersion
 	return nil
 }
 
@@ -199,6 +202,14 @@ func (km *KeyMgr) GenerateKeyset() (*model.Keyset, error) {
 	}, nil
 }
 
+// getSecretPath constructs the Vault secret path for storing keys based on the KV version.
+func (km *KeyMgr) getSecretPath(keyID string) string {
+	if km.KvVersion == "v2" {
+		return fmt.Sprintf("secret/data/keys/%s", keyID)
+	}
+	return fmt.Sprintf("secret/keys/%s", keyID)
+}
+
 // InsertKeyset stores the given keyset in Vault under the specified key ID.
 func (km *KeyMgr) InsertKeyset(ctx context.Context, keyID string, keys *model.Keyset) error {
 	if keyID == "" {
@@ -215,19 +226,17 @@ func (km *KeyMgr) InsertKeyset(ctx context.Context, keyID string, keys *model.Ke
 		"encrPublicKey":     keys.EncrPublic,
 		"encrPrivateKey":    keys.EncrPrivate,
 	}
-	var path string
+	path := km.getSecretPath(keyID)
 	var payload map[string]interface{}
 	if km.KvVersion == "v2" {
-		path = fmt.Sprintf("secret/data/keys/%s", keyID)
 		payload = map[string]interface{}{"data": keyData}
 	} else {
-		path = fmt.Sprintf("secret/keys/%s", keyID)
 		payload = keyData
 	}
 
 	_, err := km.VaultClient.Logical().Write(path, payload)
 	if err != nil {
-		return fmt.Errorf("failed to store secret in Vault: %w", err)
+		return fmt.Errorf("failed to store secret in Vault at path %s: %w", path, err)
 	}
 	return nil
 }
@@ -237,12 +246,7 @@ func (km *KeyMgr) DeleteKeyset(ctx context.Context, keyID string) error {
 	if keyID == "" {
 		return ErrEmptyKeyID
 	}
-	var path string
-	if km.KvVersion == "v2" {
-		path = fmt.Sprintf("secret/data/private_keys/%s", keyID)
-	} else {
-		path = fmt.Sprintf("secret/private_keys/%s", keyID)
-	}
+	path := km.getSecretPath(keyID)
 	return km.VaultClient.KVv2(path).Delete(ctx, keyID)
 }
 
@@ -252,12 +256,7 @@ func (km *KeyMgr) Keyset(ctx context.Context, keyID string) (*model.Keyset, erro
 		return nil, ErrEmptyKeyID
 	}
 
-	var path string
-	if km.KvVersion == "v2" {
-		path = fmt.Sprintf("secret/data/private_keys/%s", keyID)
-	} else {
-		path = fmt.Sprintf("secret/private_keys/%s", keyID)
-	}
+	path := km.getSecretPath(keyID)
 
 	secret, err := km.VaultClient.Logical().Read(path)
 	if err != nil || secret == nil {

pkg/plugin/implementation/keymanager/keymanager_test.go

@@ -636,7 +636,7 @@ func TestStorePrivateKeysFailure(t *testing.T) {
 			keyID:       "mykeyid",
 			keys:        keys,
 			statusCode:  500,
-			expectedErr: "failed to store secret in Vault: Error making API request",
+			expectedErr: "failed to store secret in Vault at path secret/keys/mykeyid: Error making API request.",
 		},
 	}
 
@@ -699,14 +699,14 @@ func TestDeletePrivateKeys(t *testing.T) {
 			name:      "v1 delete",
 			kvVersion: "v1",
 			keyID:     "key123",
-			wantPath:  "/v1/secret/private_keys/key123/data/key123",
+			wantPath:  "/v1/secret/keys/key123/data/key123",
 			wantErr:   nil,
 		},
 		{
 			name:      "v2 delete",
 			kvVersion: "v2",
 			keyID:     "key123",
-			wantPath:  "/v1/secret/data/private_keys/key123/data/key123",
+			wantPath:  "/v1/secret/data/keys/key123/data/key123",
 			wantErr:   nil,
 		},
 	}
@@ -759,9 +759,8 @@ func setupMockVaultServer(t *testing.T, kvVersion, keyID string, success bool) *
 	t.Helper()
 
 	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// Check that request path is expected
-		expectedPathV1 := fmt.Sprintf("/v1/secret/private_keys/%s", keyID)
-		expectedPathV2 := fmt.Sprintf("/v1/secret/data/private_keys/%s", keyID)
+		expectedPathV1 := fmt.Sprintf("/v1/secret/keys/%s", keyID)
+		expectedPathV2 := fmt.Sprintf("/v1/secret/data/keys/%s", keyID)
 
 		if (kvVersion == "v2" && r.URL.Path != expectedPathV2) || (kvVersion != "v2" && r.URL.Path != expectedPathV1) {
 			http.Error(w, "not found", http.StatusNotFound)
@@ -769,14 +768,18 @@ func setupMockVaultServer(t *testing.T, kvVersion, keyID string, success bool) *
 		}
 
 		if !success {
-			// Simulate Vault error or not found
 			http.Error(w, `{"errors":["key not found"]}`, http.StatusNotFound)
 			return
 		}
 
-		// Success response JSON, different for v1 and v2
+		w.Header().Set("Content-Type", "application/json")
+
 		if kvVersion == "v2" {
 			resp := fmt.Sprintf(`{
+				"request_id": "req-1234",
+				"lease_id": "",
+				"renewable": false,
+				"lease_duration": 0,
 				"data": {
 					"data": {
 						"uniqueKeyID": "%s",
@@ -784,27 +787,35 @@ func setupMockVaultServer(t *testing.T, kvVersion, keyID string, success bool) *
 						"signingPrivateKey": "sign-priv",
 						"encrPublicKey": "encr-pub",
 						"encrPrivateKey": "encr-priv"
+					},
+					"metadata": {
+						"created_time": "2025-05-28T00:00:00Z",
+						"deletion_time": "",
+						"destroyed": false,
+						"version": 1
 					}
-				}
+				},
+				"warnings": null,
+				"auth": null
 			}`, keyID)
-			w.Header().Set("Content-Type", "application/json")
-			if _, err := w.Write([]byte(resp)); err != nil {
-				t.Fatalf("failed to write response: %v", err)
-			}
+			w.Write([]byte(resp))
 		} else {
 			resp := fmt.Sprintf(`{
+				"request_id": "req-1234",
+				"lease_id": "",
+				"renewable": false,
+				"lease_duration": 0,
 				"data": {
 					"uniqueKeyID": "%s",
 					"signingPublicKey": "sign-pub",
 					"signingPrivateKey": "sign-priv",
 					"encrPublicKey": "encr-pub",
 					"encrPrivateKey": "encr-priv"
-				}
+				},
+				"warnings": null,
+				"auth": null
 			}`, keyID)
-			w.Header().Set("Content-Type", "application/json")
-			if _, err := w.Write([]byte(resp)); err != nil {
-				t.Fatalf("failed to write response: %v", err)
-			}
+			w.Write([]byte(resp))
 		}
 	})