-
Notifications
You must be signed in to change notification settings - Fork 56
Add support for BLS keys and BBS+ signatures #288
Conversation
Codecov Report
@@ Coverage Diff @@
## main #288 +/- ##
==========================================
- Coverage 58.68% 57.31% -1.37%
==========================================
Files 42 46 +4
Lines 4717 5467 +750
==========================================
+ Hits 2768 3133 +365
- Misses 1462 1759 +297
- Partials 487 575 +88
|
Co-authored-by: Andres Uribe <[email protected]>
* origin/bbs-plus: Update cryptosuite/bbsplussignaturesuite.go # Conflicts: # cryptosuite/bbsplussignaturesuite.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL - sorry for long review cycle.
// helpers | ||
|
||
func prepareBBSMessage(msg []byte) [][]byte { | ||
rows := strings.Split(string(msg), "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - weird... the link seems to be broken for me. Something must be up :/
cryptosuite/bbsplussignaturesuite.go
Outdated
b.ProofValue = proofValue | ||
} | ||
|
||
func (b *BBSPlusSignature2020Proof) ToGenericProof() (crypto.Proof, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this - was the documentation added elsewhere?
cryptosuite/bls12381g2key2020.go
Outdated
} | ||
|
||
// GenerateBLSKey2020 https://w3c-ccg.github.io/ldp-bbs2020/#bls-12-381-g2-public-key | ||
func GenerateBLSKey2020() (*BLSKey2020, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is creating a G2, should it be named appropriately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kept the name and added a param, also updated broken link
} | ||
|
||
// CreateDeriveProof https://w3c-ccg.github.io/ldp-bbs2020/#create-derive-proof-data-algorithm | ||
func (b BBSPlusSignatureProofSuite) CreateDeriveProof(inputProofDocument Provable, revealDocument map[string]interface{}) (*DeriveProofResult, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the inputProofDocument
need to be of type Provable
? Should it be any
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to any
// orig | ||
p, _ := util.PrettyJSON(testCred) | ||
println(string(p)) | ||
|
||
// frame | ||
p, _ = util.PrettyJSON(revealDoc) | ||
println(string(p)) | ||
|
||
// // reveal | ||
p, _ = util.PrettyJSON(selectiveDisclosure) | ||
println(string(p)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the println
statements intentional? I'm guessing you wanted to test that the reveal
does not contain credentialSubject
. If so, can you encode that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for the demo I gave, removing
Co-authored-by: Andres Uribe <[email protected]>
Co-authored-by: Andres Uribe <[email protected]>
Co-authored-by: Andres Uribe <[email protected]>
* origin/main: Bump github.com/multiformats/go-multibase from 0.1.1 to 0.2.0 (#313) Bump github.com/go-playground/validator/v10 from 10.11.2 to 10.12.0 (#311) Bump github.com/goccy/go-json from 0.10.0 to 0.10.2 (#310) Bump golang.org/x/term from 0.5.0 to 0.6.0 (#299) Update JWX lib to use v2 (#308) Added style and best practices (#298) Add models for Credential Issuer Metadata (#304) Upgrade go version to 1.20.2 (#305) add missing param (#297) interface to any (#296) # Conflicts: # cryptosuite/cryptosuite.go # cryptosuite/jsonwebkey2020.go # cryptosuite/jwssignaturesuite.go # cryptosuite/jwssignaturesuite_test.go # go.mod # go.sum # util/helpers.go # wasm/static/main.wasm
@andresuribe87 should have addressed all comments |
) | ||
|
||
type BBSPlusSignatureProofSuite struct { | ||
CryptoSuiteProofType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a misunderstanding there. Embedding does not force implementation of the interface. Instead, you are saying that the BBSPlusSignatureProofSuite
has an element inside it that implements that interface. Said another way, BBSPlusSignatureProofSuite.CryptoSuiteProofType
will always be nil
, since GetBBSPlusSignatureProofSuite()
does not instantiate it. See the example below
type suite struct {
fooInter
}
type fooInter interface{}
func GetSuite() *suite {
return new(suite)
}
func TestFoo(t *testing.T) {
assert.Nil(t, GetSuite().fooInter)
}
I think that's different from your intention, which is that BBSPlusSignatureProofSuite
should comply with the interface. That is achieved with line 325 alone.
cryptosuite/bbsplussignaturesuite.go
Outdated
BBSPlusSignatureSuiteCanonicalizationAlgorithm string = "https://w3id.org/security#URDNA2015" | ||
// BBSPlusSignatureSuiteDigestAlgorithm uses https://www.rfc-editor.org/rfc/rfc4634 | ||
BBSPlusSignatureSuiteDigestAlgorithm gocrypto.Hash = gocrypto.BLAKE2b_384 | ||
// BBSPlusSignatureSuiteProofAlgorithm uses https://www.rfc-editor.org/rfc/rfc7797 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no const named BBSPlusSignatureSuiteProofAlgorithm
. Is that intentional?
cryptosuite/bbsplussignaturesuite.go
Outdated
) | ||
|
||
type BBSPlusSignatureSuite struct { | ||
CryptoSuiteProofType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, I think this should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
cryptosuite/bbsplussignaturesuite.go
Outdated
|
||
var _ CryptoSuiteProofType = (*BBSPlusSignatureSuite)(nil) | ||
|
||
func (BBSPlusSignatureSuite) Marshal(data interface{}) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer any
over interface{}
. I think you had already done that in a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this pr predated the other, will update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! One minor reminder
) | ||
|
||
type BBSPlusSignatureProofSuite struct { | ||
CryptoSuiteProofType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to do this.
github.com/klauspost/cpuid/v2 v2.0.9 // indirect | ||
github.com/hyperledger/aries-framework-go/spi v0.0.0-20221025204933-b807371b6f1e // indirect | ||
github.com/kilic/bls12-381 v0.1.1-0.20210503002446-7b7597926c69 // indirect | ||
github.com/kr/text v0.2.0 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks sus haha. Who's using this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github.com/hyperledger/[email protected] github.com/klauspost/[email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like they have widely used packages https://github.com/klauspost/compress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing some CPUs can't generate these keys properly https://github.com/klauspost/cpuid but this is an indirect dep even for aries so im not certain
Fixes #27