Skip to content

Conversation

@adecaro
Copy link
Contributor

@adecaro adecaro commented Jul 19, 2022

Signed-off-by: Angelo De Caro adc@zurich.ibm.com



Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
…ssuer #299

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
Comment on lines 106 to 109
b, _ := pem.Decode(bytes)
if b == nil { // TODO: also check that the type is what we expect (cert vs key..)
return nil, errors.Errorf("no pem content for file %s", file)
}
Copy link
Member

Choose a reason for hiding this comment

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

Here you just want to check if bytes contains at least a valid pem decoded block, right? What is multiple blocks are available?

Comment on lines 53 to 55
if err != nil {
return nil, errors.Wrapf(err, "failed to load certificates from %s", signcertDir)
}
Copy link
Member

Choose a reason for hiding this comment

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

redundant error check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the folder might contain no files

Copy link
Member

Choose a reason for hiding this comment

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

What I am saying is that we check the error twice! in line 52 and 58.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, sorry


"github.com/hyperledger-labs/fabric-smart-client/platform/fabric/core/generic/msp/x509"
msp3 "github.com/hyperledger/fabric/msp"

Copy link
Member

Choose a reason for hiding this comment

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

empty line

"github.com/hyperledger-labs/fabric-token-sdk/token/core/cmd/pp/common"

"github.com/hyperledger-labs/fabric-token-sdk/token/core/zkatdlog/crypto"

Copy link
Member

Choose a reason for hiding this comment

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

empty lines

…ssuer #299

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro adecaro requested a review from mbrandenburger July 19, 2022 08:02
Comment on lines 53 to 55
if err != nil {
return nil, errors.Wrapf(err, "failed to load certificates from %s", signcertDir)
}
Copy link
Member

Choose a reason for hiding this comment

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

What I am saying is that we check the error twice! in line 52 and 58.

return fileCont, nil
}

func ReadPemFile(file string) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's give the function some documentation and even change the name. ReadPemFile is a very generic name, however, the current impl returns errors if the content in the pem file it not a cert, or there are more than one entry.

Maybe call it ReadCertFromPemFile(file string) explaining that it returns the first item from a pem file if exists. If needed we can later add ReadCertsFromPemFile(...) to parse the entire file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

Comment on lines +109 to +111
if len(rest) != 0 {
return nil, errors.Errorf("extra content after pem file %s", file)
}
Copy link
Member

Choose a reason for hiding this comment

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

We could just ignore the rest. If there are more certs in the file, we don't care, do we?

Copy link
Contributor Author

@adecaro adecaro Jul 19, 2022

Choose a reason for hiding this comment

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

I would say, let's constrain to a single certificate in the file

return nil, errors.Errorf("pem file %s is not a certificate", file)
}

return bytes, nil
Copy link
Member

Choose a reason for hiding this comment

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

should we just return b?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we force only one cert, we can return directly the bytes

…ssuer #299

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro adecaro requested a review from mbrandenburger July 19, 2022 09:08
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

LGTM

@adecaro adecaro merged commit a66a1f6 into main Jul 19, 2022
@adecaro adecaro deleted the f-299 branch July 19, 2022 11:36
HagarMeir pushed a commit that referenced this pull request Jul 25, 2022
 (#300)

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
mbwhite pushed a commit to mbwhite/fabric-token-sdk that referenced this pull request Oct 19, 2022
- Update readme with link to gopath doc
- Enforce GOPATH is set when using integration test suite

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tokengen expects a full msp directory, but should also work without signing key of auditor and issuer

3 participants