fix(idemix): never return a nil error when credential verification fails

Signed-off-by: AkramBitar <akram@il.ibm.com>

Author: AkramBitar

docs/services/identity.md

@@ -268,6 +268,14 @@ To accommodate different deployment structures, the Key Manager performs directo
 1. It first attempts to load the files directly from the configured directory (`<dir>`).
 2. If this fails, it appends an extra `msp` path element to the directory (i.e., `<dir>/msp/`) and tries again (e.g. searching for `<dir>/msp/msp/IssuerPublicKey` and `<dir>/msp/user/SignerConfig`).
 
+##### Credential Verification at Load Time
+When the loaded signer configuration carries secret key material (user secret key plus credential),
+the Idemix Key Manager verifies the credential against the issuer public key while it is being
+constructed. A credential that does not verify — whether the underlying BCCSP reports the failure as
+an error or simply as a negative verification result — makes construction fail with
+`credential is not cryptographically valid`; no key manager is returned. Configurations without
+secret key material are loaded as verify-only (remote) key managers and skip this check.
+
 #### 3. IdemixNym (Idemix with Pseudonym-based Identity)
 An extension of Idemix that uses a **commitment to the Enrollment ID (EID)** as the identity instead of the full Idemix signature.
 *   **Identity (Payload)**: A small **Nym EID** (a cryptographic commitment to the enrollment ID, $g^{sk} \cdot h^{r_{eid}}$).

token/services/identity/idemix/km.go

@@ -167,9 +167,16 @@ func NewKeyManagerWithSchema(
 				},
 			},
 		)
-		if err != nil || !valid {
+		// Keep the two failure modes apart: the BCCSP may report an invalid credential either by
+		// returning an error or by returning valid == false with a nil error. Wrapping a nil error
+		// yields a nil error, which would turn a verification failure into a (nil, nil) return and
+		// a nil-pointer panic in the caller.
+		if err != nil {
 			return nil, errors.WithMessagef(err, "credential is not cryptographically valid")
 		}
+		if !valid {
+			return nil, errors.New("credential is not cryptographically valid")
+		}
 		logger.Debugf("the signer contains key material, load it, done.")
 	} else {
 		logger.Debugf("the signer does not contain full key material, it will be considered remote [cred=%d,sk=%d]", len(conf.Signer.Cred), len(conf.Signer.Sk))

token/services/identity/idemix/km_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/LFDT-Panurus/panurus/token/services/logging"
 	kvs2 "github.com/LFDT-Panurus/panurus/token/services/storage/db/kvs"
 	"github.com/LFDT-Panurus/panurus/token/services/utils"
+	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
 	_ "github.com/hyperledger-labs/fabric-smart-client/platform/view/services/storage/driver/memory"
 	"github.com/hyperledger-labs/fabric-smart-client/platform/view/services/view"
 	"github.com/stretchr/testify/assert"
@@ -971,3 +972,146 @@ func testKeyManager_DeserializeSigningIdentityNoProbeRemote(t *testing.T, config
 	_, err = remoteKM.DeserializeSigningIdentityNoProbe(t.Context(), identityDescriptor.Identity)
 	require.Error(t, err)
 }
+
+// verifyOverridingCSP decorates a real bccsp.BCCSP and forces the outcome of credential
+// verification, so tests can exercise both ways the BCCSP reports a bad credential: a plain
+// `false` with no error, and a `false` accompanied by an error.
+type verifyOverridingCSP struct {
+	types.BCCSP
+	valid     bool
+	verifyErr error
+}
+
+func (c *verifyOverridingCSP) Verify(k types.Key, signature, digest []byte, opts types.SignerOpts) (bool, error) {
+	if _, ok := opts.(*types.IdemixCredentialSignerOpts); ok {
+		return c.valid, c.verifyErr
+	}
+
+	return c.BCCSP.Verify(k, signature, digest, opts)
+}
+
+// TestNewKeyManagerCredentialVerificationFailure is a regression test for the case where the BCCSP
+// reports a cryptographically invalid credential through its boolean return value alone
+// (valid == false, err == nil). The constructor used to funnel both failure modes through
+// errors.WithMessagef(err, ...), which returns nil when the wrapped error is nil, so it returned
+// (nil, nil): callers checking only err != nil went on to use a nil *KeyManager and panicked
+// later, away from the real cause.
+func TestNewKeyManagerCredentialVerificationFailure(t *testing.T) {
+	for _, curve := range []struct {
+		configPath string
+		curveID    math.CurveID
+	}{
+		{"./testdata/bls12_381_bbs_gurvy/idemix", math.BLS12_381_BBS_GURVY},
+		{"./testdata/bls12_381_bbs/idemix", math.BLS12_381_BBS_GURVY},
+	} {
+		for _, tc := range []struct {
+			name      string
+			valid     bool
+			verifyErr error
+			// contains lists substrings the returned error must mention
+			contains []string
+		}{
+			{
+				// the bug: failure signalled by the boolean only
+				name:     "invalid credential, no error from the BCCSP",
+				valid:    false,
+				contains: []string{"credential is not cryptographically valid"},
+			},
+			{
+				name:      "invalid credential, error from the BCCSP",
+				valid:     false,
+				verifyErr: errors.New("verification exploded"),
+				contains:  []string{"credential is not cryptographically valid", "verification exploded"},
+			},
+			{
+				// an error alongside valid == true is still a failure
+				name:      "error from the BCCSP with valid set",
+				valid:     true,
+				verifyErr: errors.New("verification exploded"),
+				contains:  []string{"credential is not cryptographically valid", "verification exploded"},
+			},
+		} {
+			t.Run(tc.name+" ["+curve.configPath+"]", func(t *testing.T) {
+				kvs, err := kvs2.NewInMemory()
+				require.NoError(t, err)
+				config, err := crypto.NewConfig(curve.configPath)
+				require.NoError(t, err)
+				keyStore, err := crypto.NewKeyStore(curve.curveID, kvs2.Keystore(kvs))
+				require.NoError(t, err)
+				realCSP, err := crypto.NewBCCSP(keyStore, curve.curveID)
+				require.NoError(t, err)
+
+				keyManager, err := NewKeyManager(
+					config,
+					types.EidNymRhNym,
+					&verifyOverridingCSP{BCCSP: realCSP, valid: tc.valid, verifyErr: tc.verifyErr},
+				)
+				// the invariant callers rely on: a failure is never reported as (nil, nil)
+				require.Error(t, err, "credential verification failure must return a non-nil error")
+				require.Nil(t, keyManager)
+				for _, substring := range tc.contains {
+					require.ErrorContains(t, err, substring)
+				}
+			})
+		}
+	}
+}
+
+// TestNewKeyManagerCredentialVerificationSuccess pins the happy path through the same decorator,
+// proving the failure cases above are caused by the verification outcome and not by the decorator
+// itself.
+func TestNewKeyManagerCredentialVerificationSuccess(t *testing.T) {
+	const configPath = "./testdata/bls12_381_bbs_gurvy/idemix"
+	kvs, err := kvs2.NewInMemory()
+	require.NoError(t, err)
+	config, err := crypto.NewConfig(configPath)
+	require.NoError(t, err)
+	keyStore, err := crypto.NewKeyStore(math.BLS12_381_BBS_GURVY, kvs2.Keystore(kvs))
+	require.NoError(t, err)
+	realCSP, err := crypto.NewBCCSP(keyStore, math.BLS12_381_BBS_GURVY)
+	require.NoError(t, err)
+
+	keyManager, err := NewKeyManager(
+		config,
+		types.EidNymRhNym,
+		&verifyOverridingCSP{BCCSP: realCSP, valid: true},
+	)
+	require.NoError(t, err)
+	require.NotNil(t, keyManager)
+}
+
+// TestNewKeyManagerTamperedCredential drives the same failure end to end, with no test double: a
+// byte of the real credential is flipped so the real BCCSP rejects it. Whichever way the BCCSP
+// signals the rejection, construction must fail loudly rather than hand back a nil key manager.
+func TestNewKeyManagerTamperedCredential(t *testing.T) {
+	testNewKeyManagerTamperedCredential(t, "./testdata/bls12_381_bbs_gurvy/idemix", math.BLS12_381_BBS_GURVY)
+	testNewKeyManagerTamperedCredential(t, "./testdata/bls12_381_bbs/idemix", math.BLS12_381_BBS_GURVY)
+}
+
+func testNewKeyManagerTamperedCredential(t *testing.T, configPath string, curveID math.CurveID) {
+	t.Helper()
+	kvs, err := kvs2.NewInMemory()
+	require.NoError(t, err)
+	config, err := crypto.NewConfig(configPath)
+	require.NoError(t, err)
+	keyStore, err := crypto.NewKeyStore(curveID, kvs2.Keystore(kvs))
+	require.NoError(t, err)
+	cryptoProvider, err := crypto.NewBCCSP(keyStore, curveID)
+	require.NoError(t, err)
+
+	// sanity check: the untouched credential is accepted
+	keyManager, err := NewKeyManager(config, types.EidNymRhNym, cryptoProvider)
+	require.NoError(t, err)
+	require.NotNil(t, keyManager)
+
+	// tamper with the credential's signature material
+	config, err = crypto.NewConfig(configPath)
+	require.NoError(t, err)
+	require.NotEmpty(t, config.Signer.Cred)
+	config.Signer.Cred[len(config.Signer.Cred)-1] ^= 0xFF
+
+	keyManager, err = NewKeyManager(config, types.EidNymRhNym, cryptoProvider)
+	require.Error(t, err, "a tampered credential must return a non-nil error")
+	require.Nil(t, keyManager)
+	require.ErrorContains(t, err, "credential is not cryptographically valid")
+}