feat: Update Tink version, protobuf type #3413

DRK3 · 2022-10-21T21:46:56Z

Updated the Tink version and our methods to use the newer protobuf type that it uses.

Reason for this change: the existing version of Tink makes use of a very large int in one particular place in the code, and this very large int prevents the gomobile tool from successfully generating Android bindings. This meant that aries-framework-go packages that used Tink could not be used in any code that you may want to generate Android bindings for. The updated version of Tink does not have that check anymore, which resolves the issue.

Due to a circular dependency between the main module and component/storage/edv (and the fact that both of them require Tink), there are some module/build issues with this commit. This will be resolved in the next commit.

Signed-off-by: Derek Trider [email protected]

DRK3 · 2022-10-25T01:43:35Z

pkg/crypto/tinkcrypto/primitive/secp256k1/secp256k1_signer_key_manager_test.go

@@ -278,14 +278,6 @@ func TestPublicKeyDataWithInvalidInput(t *testing.T) {

 	_, err = pkm.PublicKeyData(serializedKey)
 	require.Error(t, err, "expect an error when input is a modified serialized key")
-
-	// nil
-	_, err = pkm.PublicKeyData(nil)


No longer returns an error with the updated Tink version (pub key bytes in returned struct are just empty now)

you can remove the whole test function since the main test is not applicable anymore

@Baha-sk You mean remove TestPublicKeyDataWithInvalidInput? Are the other test cases in it not applicable?

yes, the whole test function's purpose was to test empty/nil cases, it seems Tink now panics on purpose in these cases... you can probably recover the panic here and make sure it does actually panic instead of removing these calls? that would be a good test case.

There are some other test cases in this function still that work the same as before (above the code I removed, in this same function). For these two particular test cases I removed, they just don't return an error anymore (i.e. are not an invalid test case anymore)

Good point re: catching the panic - I'll update the other places in this PR where I removed those tests that panic to catch it

There are some other test cases in this function still that work the same as before (above the code I removed, in this same function). For these two particular test cases I removed, they just don't return an error anymore (i.e. are not an invalid test case anymore)

I think you can use require.Panics() in this case, without having to set a value.

These two cases don't panic - they're not an error or panic

oh ok.. I see.. no issues then.. it looks good.

DRK3 · 2022-10-25T01:44:23Z

pkg/crypto/tinkcrypto/primitive/secp256k1/secp256k1_signer_key_manager_test.go

-	require.Errorf(t, err, "expect an error when input is nil")
-
-	// empty slice
-	_, err = pkm.PublicKeyData([]byte{})


No longer returns an error with the updated Tink version (pub key bytes in returned struct are just empty now)

DRK3 · 2022-10-25T01:48:00Z

pkg/crypto/tinkcrypto/unwrap_support_test.go

@@ -38,9 +38,6 @@ func Test_ExtractPrivKey(t *testing.T) {
 	require.EqualError(t, err, "extractPrivKey: can't extract unsupported private key 'type.googleapis.com/"+
 		"google.crypto.tink.AesGcmKey'")

-	_, err = extractPrivKey(&keyset.Handle{})


Passing an empty keyset handle causes Tink to do a panic now instead of throwing an error

ok, this is fine then

Updated to check for the panic now using require.PanicsWithValue

Moopli · 2022-10-25T14:33:45Z

Why do we need a second PR for the second commit?
One PR with two commits should allow tests to pass once the PR is merged.

DRK3 · 2022-10-25T15:19:10Z

@Moopli I don't have a valid commit I can use with go get until this is merged into the main branch of hyperledger/aries-framework-go. Unless you're talking about manually editing the go.mod file ahead of time with what go get would have done? In which case, I'm not sure how I would generate the date string (the entry would look something like v0.1.8-????????-33c163460841, and I'm sure the go.sum and other go modules in Aries would need to be updated anyway, and I think we need the go tools + the commit merged in for that). Let me know if you mean something else!

baha-ai · 2022-10-25T16:12:07Z

One PR with two commits should allow tests to pass once the PR is merged.

you can push a second commit within the same PR where you can use the commit hash of the first commit in go.mod of the second commit, @Moopli is right

n which case, I'm not sure how I would generate the date string (the entry would look something like v0.1.8-????????-33c163460841, and I'm sure the go.sum and other go modules in Aries would need to be updated anyway, and I think we need the go tools + the commit merged in for that). Let me know if you mean something else!

just replace the whole commit version with the sha hash of the first commit in go.mod then do go mod tidy, it will automatically update go.mod and go.sum for you with the right version

baha-ai · 2022-10-25T16:31:41Z

never mind my previous comment, git needs to query GH to fetch the commit, and it's not there till it's merged

Updated the Tink version and our methods to use the newer protobuf type that it uses. Reason for this change: the existing version of Tink makes use of a very large int in one particular place in the code, and this very large int prevents the gomobile tool from successfully generating Android bindings. This meant that aries-framework-go packages that used Tink could not be used in any code that you may want to generate Android bindings for. The updated version of Tink does not have that check anymore, which resolves the issue. Due to a circular dependency between the main module and component/storage/edv (and the fact that both of them require Tink), there are some module/build issues with this commit. This will be resolved in the next commit. Signed-off-by: Derek Trider <[email protected]>

DRK3 force-pushed the UpdateTinkVersion branch 12 times, most recently from c346ec0 to 0f76497 Compare October 25, 2022 01:38

DRK3 commented Oct 25, 2022

View reviewed changes

DRK3 marked this pull request as ready for review October 25, 2022 14:26

DRK3 requested a review from baha-ai October 25, 2022 14:26

DRK3 force-pushed the UpdateTinkVersion branch 2 times, most recently from 4a9fae0 to 33c1634 Compare October 25, 2022 14:56

baha-ai approved these changes Oct 25, 2022

View reviewed changes

DRK3 force-pushed the UpdateTinkVersion branch from 33c1634 to 253124a Compare October 25, 2022 16:36

Moopli approved these changes Oct 25, 2022

View reviewed changes

rolsonquadras approved these changes Oct 25, 2022

View reviewed changes

sudeshrshetty approved these changes Oct 25, 2022

View reviewed changes

fqutishat merged commit b807371 into hyperledger-archives:main Oct 25, 2022

DRK3 mentioned this pull request Oct 26, 2022

Switch github.com/golang/protobuf to google.golang.org/protobuf #3420

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update Tink version, protobuf type #3413

feat: Update Tink version, protobuf type #3413

DRK3 commented Oct 21, 2022 •

edited

Loading

DRK3 Oct 25, 2022

baha-ai Oct 25, 2022

DRK3 Oct 25, 2022

baha-ai Oct 25, 2022

DRK3 Oct 25, 2022 •

edited

Loading

DRK3 Oct 25, 2022

baha-ai Oct 25, 2022

DRK3 Oct 25, 2022

baha-ai Oct 25, 2022

DRK3 Oct 25, 2022

DRK3 Oct 25, 2022

baha-ai Oct 25, 2022

DRK3 Oct 25, 2022

Moopli commented Oct 25, 2022

DRK3 commented Oct 25, 2022 •

edited

Loading

baha-ai commented Oct 25, 2022 •

edited

Loading

baha-ai commented Oct 25, 2022

feat: Update Tink version, protobuf type #3413

feat: Update Tink version, protobuf type #3413

Conversation

DRK3 commented Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DRK3 Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moopli commented Oct 25, 2022

DRK3 commented Oct 25, 2022 • edited Loading

baha-ai commented Oct 25, 2022 • edited Loading

baha-ai commented Oct 25, 2022

DRK3 commented Oct 21, 2022 •

edited

Loading

DRK3 Oct 25, 2022 •

edited

Loading

DRK3 commented Oct 25, 2022 •

edited

Loading

baha-ai commented Oct 25, 2022 •

edited

Loading