Skip to content

[BREAKING] Fix TX ASN1 digest#64

Merged
cendhu merged 2 commits into
hyperledger:mainfrom
liran-funaro:asn1-marshal-fix-public
Jul 9, 2025
Merged

[BREAKING] Fix TX ASN1 digest#64
cendhu merged 2 commits into
hyperledger:mainfrom
liran-funaro:asn1-marshal-fix-public

Conversation

@liran-funaro

Copy link
Copy Markdown
Contributor

Type of change

  • Bug fix
  • Test update
  • Breaking change

Description

  • Add namespace ID and version to the ASN.1 schema
  • Remove redundant marshaling when using NONE schema
    • Allows better benchmarking of non-sig related components
  • Add a test that validates the marshalling schema includes all the fields of the TX by reconstructing the TX from the marshalled data
  • Add fuzz test for the ASN.1 marshalling
  • Add a benchmark for the digest method

The tests discovered the following:

  1. The ASN.1 library may encode a string in two ways, depending on the input string.
    If it is ASCII, it encodes it as PrintableString; otherwise, it uses UTF-8.
    This may cause incompatibility with the schema, which requires a specific type. Thus, we force the type to be UTF-8.
  2. The ASN.1 marshalling cannot differentiate between nil and empty slice ([]byte{}).
    We must ensure our implementation does not make this differentiation to avoid inconsistencies.
    The current implementation parses version []byte{} as zero. If the endorser signed on nil version, an attacker can change the TX to have version zero by replacing the nil with []byte{} and go unnoticed.

To fix these issues, we:

  • Modify the ASN.1 schema to use UTF-8 (UTF8String) instead of ASCII (PrintableString)
  • Change version to be uint64.
    • Since ASN.1 doesn't support unsigned int, we use int64 and encode nil as -1.
    • Since SQL doesn't support unsigned number, we use bigint (int64) and add check for NULL and version>=0.

Related issues

Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
@liran-funaro liran-funaro added bug Something isn't working security labels Jul 6, 2025
@cendhu cendhu requested a review from Copilot July 8, 2025 07:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how transaction namespaces are marshaled and versioned in ASN.1, switches version fields from byte slices to uint64 throughout the code and database, and streamlines no-scheme signing/verifying paths while adding new tests and benchmarks.

  • Enforce UTF8 in ASN.1 schema, add namespace ID and version (using int64 with –1 default) to TxWithNamespace
  • Remove redundant dummy marshaling for “no scheme” in sign/verify APIs
  • Convert namespace/version fields from []byte to uint64 across protos, DB schemas, queries, and tests; introduce generic array helpers

Reviewed Changes

Copilot reviewed 57 out of 57 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
utils/signature/tx_schema.asn ASN.1 schema extended with namespaceID & version fields
utils/signature/block_tx.go TranslateTx and DigestTxNamespace use new schema & version conversions
service/vc/database.go Query templates & row readers updated for bigint versions and generics
api/protoqueryservice/query.proto Changed version from bytes to uint64
api/protoblocktx/block_tx.proto Changed ns_version, version fields to uint64
service/vc/create_namespace.sql New namespace table/functions use BIGINT
service/vc/init_database.sql Consolidated system tables/functions into a single SQL blob
Numerous tests Updated tests to use literal uint64 versions and new helpers
Comments suppressed due to low confidence (2)

service/verifier/verify.go:148

  • [nitpick] Returning ABORTED_MISSING_TXID for an invalid UTF-8 TxId may mislead callers. Define a dedicated status or error for invalid UTF-8 to improve clarity.
	if !utf8.ValidString(tx.Id) {

service/vc/dbinit.go:104

  • [nitpick] Using ReplaceAll on ns_table can inadvertently replace occurrences outside the placeholder. Consider a distinct marker like {{TABLE}} to avoid accidental replacements.
	return strings.ReplaceAll(tmpl, nsTableTemplateMarker, tableName)

Comment thread utils/signature/block_tx.go

@cendhu cendhu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. A few minor comments.

@@ -0,0 +1,79 @@
/*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why are we clubbing them into a single file? This add difficulties to the review process as we are both changing the existing SQL as well as combining them into a single file. Further, it changes the scope the PR too.

Why are we using only one ns_table?

After offline discussion, my suggestion is to retain single file if it helps but revert the changes made to existing placeholder for clarity while reading the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason for changing the format is that Goland (IntelliJ) IDE doesn't handle the %[1]s formatting well. It fails to parse it, so the SQL appears without syntax highlighting.

I opted to use ${NAMESPACE_ID} instead. Goland handles this kind of placeholder well.
It is a common placeholder for variables (e.g., bash), and it adds clarity as it specifies exactly what the placeholder is intended for.

Comment thread api/protoblocktx/block_tx.proto
Comment thread api/types/types.go
Comment thread integration/test/dependency_test.go Outdated
Comment thread service/coordinator/coordinator_test.go Outdated
Comment thread service/vc/database_test.go Outdated
Comment thread service/vc/preparer.go Outdated
Comment thread service/vc/test_exports.go
Comment thread service/vc/validator_test.go
Comment thread utils/signature/block_tx.go
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
@liran-funaro liran-funaro requested a review from cendhu July 9, 2025 09:58

@cendhu cendhu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.


CREATE TABLE IF NOT EXISTS ns_table
/*
This SQL file is a template for creating a new namespace.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add tmpl suffix to the file name create_namespace_tmpl.sql.

@cendhu cendhu merged commit b4bdbb6 into hyperledger:main Jul 9, 2025
10 checks passed
@liran-funaro liran-funaro deleted the asn1-marshal-fix-public branch July 9, 2025 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[tx digest] DigestTxNamespace does not encode the namespace ID and version

3 participants