hyperledger / hyperledger/fabric-x
bug(fxconfig/transaction): parseCertificateOrPublicKey silently swallows parse errors, giving misleading diagnostics on invalid PEM key files
- Dominant language
- Go
- Stars
- 64
- Forks
- 80
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 15
Description
### Summary
`parseCertificateOrPublicKey()` in `tools/fxconfig/internal/transaction/policy.go` has two silent failure paths that discard the real error and always return the same generic `"no ECDSA public key in block"` message — regardless of what actually went wrong. This makes diagnosing `--threshold-policy` file issues unnecessarily hard in practice.
---
### Affected File
**`tools/fxconfig/internal/transaction/policy.go`**
Function: `parseCertificateOrPublicKey`, called via `getPubKeyFromPemData` → `CreateThresholdPolicy`
---
### Root Cause
The function has two code paths where the real error is silently dropped:
```go
func parseCertificateOrPublicKey(blockBytes []byte) ([]byte, error) {
cert, err := x509.ParseCertificate(blockBytes)
var publicKey any
if err == nil {
if cert.PublicKey != nil && cert.PublicKeyAlgorithm == x509.ECDSA {
publicKey = cert.PublicKey
}
// ❌ Silent path 1: cert parsed fine but isn't ECDSA (e.g. RSA).
// publicKey stays nil — no context returned about the algorithm mismatch.
} else {
anyPublicKey, err2 := x509.ParsePKIXPublicKey(blockBytes)
if err2 == nil && anyPublicKey != nil {
publicKey, ok = anyPublicKey.(*ecdsa.PublicKey)
if !ok {
return nil, errors.New("public key is not a ecdsa public key")
}
}
// ❌ Silent path 2: err2 != nil (corrupt/malformed bytes).
// err2 is never propagated — falls through silently.
}
if publicKey == nil {
return nil, errors.New("no ECDSA public key in block") // same message for all failure paths
}
```
The caller `getPubKeyFromPemData` further swallows this via `continue`, so the end user sees:
no ECDSA public key in pem file
…with no indication of the actual root cause.
---
### Two Confirmed Failure Scenarios
**Scenario A — Valid non-ECDSA certificate (e.g. RSA cert)**
`x509.ParseCertificate` succeeds, but `cert.PublicKeyAlgorithm != x509.ECDSA`, so `publicKey` stays `nil`. The function returns the generic error `"no ECDSA public key in block"` with no mention that the file held a perfectly valid RSA certificate with the wrong algorithm.
**Scenario B — Corrupt or malformed PEM block bytes**
`x509.ParseCertificate` fails (expected). The fallback `x509.ParsePKIXPublicKey` also fails with a real `asn1: structure error`, but `err2` is never returned or wrapped — it falls through silently. The user still sees the same generic message, with the underlying ASN.1 error completely lost.
---
### Reproducer
Add the following tests to `tools/fxconfig/internal/transaction/policy_parse_test.go`:
```go
package transaction
import (
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"crypto/x509/pkix"
"math/big"
"strings"
"testing"
"time"
)
// Scenario A: Valid RSA certificate, wrong algorithm.
func TestParseCertificateOrPublicKey_RSACertGivesGenericError(t *testing.T) {
rsaKey, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
t.Fatalf("rsa keygen: %v", err)
}
template := &x509.Certificate{
SerialNumber: big.NewInt(1),
Subject: pkix.Name{CommonName: "test-rsa"},
NotBefore: time.Now(),
NotAfter: time.Now().Add(time.Hour),
}
certDER, err := x509.CreateCertificate(rand.Reader, template, template, &rsaKey.PublicKey, rsaKey)
if err != nil {
t.Fatalf("create cert: %v", err)
}
_, gotErr := parseCertificateOrPublicKey(certDER)
if gotErr == nil {
t.Fatal("expected an error for RSA cert, got nil")
}
if strings.Contains(gotErr.Error(), "RSA") || strings.Contains(gotErr.Error(), "algorithm") {
t.Logf("GOOD: error mentions algorithm: %v", gotErr)
} else {
t.Logf("BUG CONFIRMED (Scenario A): error is generic, root cause lost: %q", gotErr.Error())
}
}
// Scenario B: Corrupt bytes drop real parse error.
func TestParseCertificateOrPublicKey_CorruptBytesDropsRealErr(t *testing.T) {
corrupt := []byte("this is definitely not valid ASN.1 DER data")
_, gotErr := parseCertificateOrPublicKey(corrupt)
if gotErr == nil {
t.Fatal("expected error for corrupt bytes, got nil")
}
if strings.Contains(gotErr.Error(), "asn1") || strings.Contains(gotErr.Error(), "structure") {
t.Logf("GOOD: real parse error is propagated: %v", gotErr)
} else {
t.Logf("BUG CONFIRMED (Scenario B): real parse error swallowed, got generic: %q", gotErr.Error())
}
}
// Scenario C: Valid ECDSA public key should succeed (regression guard).
func TestParseCertificateOrPublicKey_ValidECDSAKeySucceeds(t *testing.T) {
ecKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
pubDER, _ := x509.MarshalPKIXPublicKey(&ecKey.PublicKey)
result, err := parseCertificateOrPublicKey(pubDER)
if err != nil {
t.Fatalf("unexpected error for valid ECDSA key: %v", err)
}
if len(result) == 0 {
t.Fatal("expected non-empty DER bytes")
}
}
// Scenario D: Valid ECDSA certificate should succeed (regression guard).
func TestParseCertificateOrPublicKey_ValidECDSACertSucceeds(t *testing.T) {
ecKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
template := &x509.Certificate{
SerialNumber: big.NewInt(2),
Subject: pkix.Name{CommonName: "test-ecdsa"},
NotBefore: time.Now(),
NotAfter: time.Now().Add(time.Hour),
}
certDER, _ := x509.CreateCertificate(rand.Reader, template, template, &ecKey.PublicKey, ecKey)
result, err := parseCertificateOrPublicKey(certDER)
if err != nil {
t.Fatalf("unexpected error for valid ECDSA cert: %v", err)
}
if len(result) == 0 {
t.Fatal("expected non-empty DER bytes")
}
}
```
Run with:
```bash
go test ./tools/fxconfig/internal/transaction -run TestParseCertificateOrPublicKey -v
```
---
### Observed Output (confirmed)
Both bugs confirmed. Scenarios C and D pass cleanly, confirming the regression guards work and valid ECDSA inputs are unaffected.
---
### Expected vs. Actual Errors
| Input | Current error | Expected error |
|---|---|---|
| Valid RSA certificate | `"no ECDSA public key in block"` | `"certificate uses RSA algorithm, expected ECDSA"` |
| Corrupt DER bytes | `"no ECDSA public key in block"` | `"failed to parse public key: asn1: structure error: ..."` |
---
### Suggested Fix
```go
func parseCertificateOrPublicKey(blockBytes []byte) ([]byte, error) {
cert, err := x509.ParseCertificate(blockBytes)
var publicKey any
if err == nil {
if cert.PublicKeyAlgorithm != x509.ECDSA {
return nil, fmt.Errorf(
"certificate uses %s algorithm, expected ECDSA",
cert.PublicKeyAlgorithm,
)
}
publicKey = cert.PublicKey
} else {
anyPublicKey, err2 := x509.ParsePKIXPublicKey(blockBytes)
if err2 != nil {
return nil, fmt.Errorf("failed to parse public key: %w", err2)
}
var ok bool
publicKey, ok = anyPublicKey.(*ecdsa.PublicKey)
if !ok {
return nil, errors.New("public key is not an ECDSA public key")
}
}
key, err := x509.MarshalPKIXPublicKey(publicKey)
if err != nil {
return nil, fmt.Errorf("marshalling public key failed: %w", err)
}
return key, nil
}
```
---
### Impact
Users running:
```bash
fxconfig namespace create --threshold-policy
fxconfig namespace update --threshold-policy
```
…who supply a wrong-algorithm certificate or a corrupted PEM file currently receive only:
no ECDSA public key in pem file
There is no indication of whether the failure was an algorithm mismatch or a structural parse error, which significantly slows down debugging.
---
### Environment
- **Repo:** `hyperledger/fabric-x`
- **File:** `tools/fxconfig/internal/transaction/policy.go`
- **Functions:** `parseCertificateOrPublicKey`, `getPubKeyFromPemData`, `CreateThresholdPolicy`
- **Verified with:** Go 1.23, branch `bug/policy-parse-errors`
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.