Added hsm samples using Gateway

[sapthasurendran]

Signed-off-by: sapthasurendran saptha.surendran@ibm.com
Moving hsm-samples from gateway repo to fabric-samples

[bestbeforetoday]

This is going to need to be runnable, so the instructions should mention how to start the test network and install chaincode (asset-transfer-basic might be a good choice), and the code slightly updated to call a suitable function on that chaincode. Currently, it relies on the sample network that was defined in the fabric-gateway repository, and the accompanying Makefile that set it up.

[bestbeforetoday]

A few changes I think are needed described in comments. Also, I suspect the HSM sample would fit better as a top-level sample, outside of the asset-transfer-basic directory, even though it uses the asset-transfer-basic sample chaincode. The off_chain_data sample also does this. The HSM sample is demonstrating the use of HSM, nothing specific to the basic asset transfer scenario. The sample applications could actually be simpler than they are too. They just need to demonstrate that an evaluate and a submit both work when an HSM signing implementation is used.

[bestbeforetoday]

go get is now only for updating module dependencies. To install the fabric-ca-client command, it should use go install.

Suggested change

	go get -tags 'pkcs11' github.com/hyperledger/fabric-ca/cmd/fabric-ca-client
	go install -tags pkcs11 github.com/hyperledger/fabric-ca/cmd/fabric-ca-client@latest

[bestbeforetoday]

This doesn't work on my Mac with softhsm2 installed using Homebrew. The problem looks to be that the "standard" PKCS11 library locations checked for by the script don't include the location for my environment. Here is a potential update to the generate-hsm-user.sh script that might help by allowing a PKCS11_LIB environment variable to define the library location:

#!/usr/bin/env bash
set -eo pipefail

# define the CA setup
CA_HOST=localhost
CA_URL=${CA_HOST}:7054
TLS_CERT='../../../test-network/organizations/fabric-ca/org1/tls-cert.pem'

LocateHsmLib() {
  if [[ -n "${PKCS11_LIB}" && -f "${PKCS11_LIB}" ]]; then
    echo "${PKCS11_LIB}"
    return
  fi

  local POSSIBLE_LIB_LOC=( \
    '/usr/lib/softhsm/libsofthsm2.so' \
    '/usr/lib/x86_64-linux-gnu/softhsm/libsofthsm2.so' \
    '/usr/local/lib/softhsm/libsofthsm2.so' \
    '/usr/lib/libacsp-pkcs11.so' \
  )
  for TEST_LIB in "${POSSIBLE_LIB_LOC[@]}"; do
    if [ -f "${TEST_LIB}" ]; then
      echo "${TEST_LIB}"
      return
    fi
  done
}

HSM2_LIB=$(LocateHsmLib)
[ -z $HSM2_LIB ] && echo No SoftHSM PKCS11 Library found, ensure you have installed softhsm2 && exit 1

# create a softhsm2.conf file if one doesn't exist
HSM2_CONF=$HOME/softhsm2.conf
[ ! -f $HSM2_CONF ] && echo directories.tokendir = /tmp > $HSM2_CONF

# Update the client config file to point to the softhsm pkcs11 library
# which must be in $HOME/softhsm directory
echo 'directory' $PWD

CLIENT_CONFIG_TEMPLATE=../ca-client-config/fabric-ca-client-config-template.yaml
CLIENT_CONFIG=../ca-client-config/fabric-ca-client-config.yaml
cp $CLIENT_CONFIG_TEMPLATE $CLIENT_CONFIG

if [[ "$OSTYPE" == "darwin"* ]]; then
  sed -i '' s+REPLACE_ME_HSMLIB+${HSM2_LIB}+g $CLIENT_CONFIG
else
  sed -i s+REPLACE_ME_HSMLIB+${HSM2_LIB}+g $CLIENT_CONFIG
fi

# create the users, remove any existing users
CRYPTO_PATH=$PWD/../crypto-material/hsm
[ -d $CRYPTO_PATH ] && rm -fr $CRYPTO_PATH

# user passed in as parameter
CAADMIN=admin
CAADMIN_PW=adminpw
HSMUSER=$1

SOFTHSM2_CONF=$HSM2_CONF fabric-ca-client enroll -c $CLIENT_CONFIG -u https://$CAADMIN:$CAADMIN_PW@$CA_URL --mspdir $CRYPTO_PATH/$CAADMIN --csr.hosts example.com --tls.certfiles ${TLS_CERT}
! SOFTHSM2_CONF=$HSM2_CONF fabric-ca-client register -c $CLIENT_CONFIG --mspdir $CRYPTO_PATH/$CAADMIN --id.name $HSMUSER --id.secret $HSMUSER --id.type client --caname ca-org1 --id.maxenrollments 0 -m example.com -u https://$CA_URL --tls.certfiles ${TLS_CERT} && echo user probably already registered, continuing
SOFTHSM2_CONF=$HSM2_CONF  fabric-ca-client enroll -c $CLIENT_CONFIG -u https://$HSMUSER:$HSMUSER@$CA_URL --mspdir $CRYPTO_PATH/$HSMUSER --csr.hosts example.com --tls.certfiles ${TLS_CERT}

With the script above on an (arm64) M1 Mac, this can be run using the following command:

PKCS11_LIB='/opt/homebrew/Cellar/softhsm/2.6.1/lib/softhsm/libsofthsm2.so' ./generate-hsm-user.sh HSMUser

The Go and Node client application code will need a similar change to check whether a valid PKCS11_LIB environment variable is specified and use that before falling back to the "standard" library locations.

[bestbeforetoday]

It would probably be nice to include the build as a prepare script as with the other Gateway samples, so it happens automatically at the end of the npm install and not require an additional command to be run.

[bestbeforetoday]

Several comments below but addressing them I can run through the sample OK so it's looking good.

For consistency with other samples, I would rename the node sub-directory to application-typescript, and the go sub-directory to application-go. Perhaps the top-level sample directory can just be hardware-security-module? Or if you want something shorter, maybe hsm or pkcs11, although I'm not sure they are necessarily obvious to people not already familiar with the concept.

[bestbeforetoday]

These are one-time initial set up step required before using HSM with a network so it might make more sense to have this before the network creation above

[bestbeforetoday]

This is one-time set up so might fit better in an install section before the section on running the sample

[bestbeforetoday]

It night be nice to make the script runnable from anywhere and having the instructions run it from the project directory to save changing directories unnecessarily

Suggested change

	cd scripts
	PKCS11_LIB='<path to PKCS11 library location>' ./generate-hsm-user.sh HSMUser
	PKCS11_LIB='<path to PKCS11 library location>' scripts/generate-hsm-user.sh HSMUser

The script would need this near the top, then use ${SCRIPT_DIR} to build absolute path names to any referenced files:

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )

For example, building an absolute path:

TLS_CERT="${SCRIPT_DIR}/../../test-network/organizations/fabric-ca/org1/tls-cert.pem"

[bestbeforetoday]

Not sure why this is here. I would suggest removing it

[bestbeforetoday]

Suggest capturing the script directory and using it to qualify all relative path names below:

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )

[bestbeforetoday]

Needs to check PKCS11_LIB environment variable in case library is in a non-standard location:

	pkcs11lib := os.Getenv("PKCS11_LIB")
	if pkcs11lib != "" {
		libraryLocations = append(libraryLocations, pkcs11lib)
	}

[bestbeforetoday]

This should mention the PKCS11_LIB environment variable to specify a non-standard library location, similar to the enroll step

[bestbeforetoday]

Suggested change

	go run -tags pkcs11 hsm-sample.go
	go run -tags pkcs11 .

[bestbeforetoday]

Need to consider PKCS11_LIB environment variable is set to specify a non-standard library location.

    const pkcs11lib = process.env['PKCS11_LIB'];
    if (pkcs11lib) {
        commonSoftHSMPathNames.push(pkcs11lib);
    }

[bestbeforetoday]

I would have the npm install just run the TypeScript compile, not the linting.

Suggested change

	"prepare": "npm run build",
	"build": "npm-run-all clean compile lint",
	"clean": "rimraf dist",
	"compile": "tsc",
	"build": "tsc",
	"prepare": "npm run build",
	"clean": "rimraf dist",

[bestbeforetoday]

Some specific issues in the README mentioned below. It might be possible to tweak the structure slightly for clarity but generally looks good. The sample applications ran fine for me following the README instructions, except for the malformed Go command.

[bestbeforetoday]

This is a malformed command that does not run. The PKCS11_LIB=... portion is a shell command that sets an environment variable for the following command. The command should be go run -tags pkcs11 .

[bestbeforetoday]

Seeing setting the PKCS11_LIB environment mentioned multiple times and multiple different ways in this document, I suspect it would be simpler to just mention it once here and have them set the environment variable for the shell so that it applies to all the following commands run in the shell. Then no need to mention the environment variable in the sections describing how to run each language version of the sample.

Suggested change

	If the "standard" PKCS11 library locations checked for by the script don't include the library(libsofthsm2.so) location for your environment set the `PKCS11_LIB` environment variable to define the library location.
	```bash
	PKCS11_LIB='<path to PKCS11 library location>' scripts/generate-hsm-user.sh HSMUser
	```
	This will register a user `HSMUser` with the CA in Org1 (if not already registered) and then enroll that user which will
	generate a certificate on the file system for use by the sample. The private key is stored in SoftHSM
	If your PKCS11 library (libsofthsm2.so) is not located in one of the typical Linux system locations checked by this sample's scripts and applications, you will need to explicitly specify the library location using the `PKCS11_LIB` environment variable.
	```bash
	export PKCS11_LIB='<path to PKCS11 library location>'```
	Register a user `HSMUser` with the CA in Org1 (if not already registered) and then enroll that user which will
	generate a certificate on the file system for use by the sample. The private key is stored in SoftHSM
	```bash
	scripts/generate-hsm-user.sh HSMUser```

[bestbeforetoday]

Suggested change

	## Cleanup

[bestbeforetoday]

Should also call hsmSignerFactory.dispose()

[bestbeforetoday]

The flow here is a little bit suspect. If a failure happens before the try block, some resources may not get closed. You could specify the variables that may need closing up-front (using let), then start the try block before you initialise them, then in the finally block use .? operator to only close/dispose things that got created before any failure occurred.

This is more correct but for the purposes of the sample probably doesn't matter too much as generally things will work.

[bestbeforetoday]

👍

[jkneubuh]

This is a great PR / sample. Very clear. Very smooth instructions with clear outcomes. I love the inclusion of a CI test.

[jkneubuh]

love it. This really makes it easy for the user as a supplement to the README.

[jkneubuh]

Would you consider adding a sentence here that describes, very briefly, that the user / enrollment in this test scenario will store private keys in the HSM? This should be clear from the context of a "hardware-security-module" sample, but I'd appreciate seeing this highlighted right at the top of the README. I.e.., something to the effect of : "when you use an HSM, private keys for a Fabric enrollment are stored within a dedicated hardware module, rather than in plain text on a local file system."

This doesn't need a lot of word smithing. Just a nice reminder right at the start of the overall outcome of the lesson / sample.

[jkneubuh]

This was my only stumbling block when reviewing the PR. I had tried running with the default fabric-ca-client 1.5.5 binaries, installed with the bootstrap.sh installer to a local scratch / bin directory. Either add a note here that the client binary installed to GOROOT with -tags .... should be added to the PATH, or change generate-hsm-user.sh script to explicitly call the installed client from GOROOT.

[sapthasurendran]

GOROOT is automatically added to the PATH right ?

[jkneubuh]

I really like this PR.

Reading through this code marks the first time I really understood anything about HSM. As a side-effect of working through the sample, I felt like I had an increased understanding of HSM, client certificates, and how this applies to production environments where the hardware module is available at runtime. In other words, I learned from reading through this sample. Well done!

I approve this PR, but will hold on merging it until @bestbeforetoday and @sapthasurendran give the green light on landing. I have some very minor feedback points, which are suggestions, not "must address" issues before merging. If you want to merge "as is," I'm happy to do so - just give the green light and I'll push the merge button.

In general, there are a couple of very minor points, inline below. The other ask in here is that the code be structured a little bit to support running with the certificate layout and org names as generated by the kube test network and operator based sample-network, which use slightly different host / domain names / ports for service URLs, and paths for the location of enrollment and TLS certificates.

That said, I'm 100% OK with merging this to run only on the compose based test network - we can postpone alignment of running samples across Compose / Kube / Operator until a later time.

Looks great. 👍

[jkneubuh]

Here were the (very) minor edits necessary to run the sample on the kube test network / operator sample network:

There is no requirement to land these in this PR. In general for the samples a few additional variables with defaults suitable for the compose test network will help when/if we align the test environments across different orchestration engines.

diff-for-k8s-test-network.txt

[jkneubuh]

Is this secondary enrollment of the CA admin with the HSM-enabled client required? Or is this a convenient way to generate the --mspdir input for the HSMUser registration?

[jkneubuh]

Apparently I have been bitten by the golang "error handling narrative" idioms.

This line would greatly be improved by realigning the arguments to the enrollment, one-per-line with \ line endings for readability.

More importantly, the ! and && echo user probably did something previously is understandable but a bit reckless. A preferred approach here would be to make the call (after formatting to avoid bleeding eyes), and check the $? error code with a little tweak here and there to the set -eo pipefail to not automatically exit the script if the user had previously registered the HSMUser.

CA client distinguishes between the error types in the return code. It would be nice to just see this in the script rather than try to infer what ! means in a script with the && ... at the end. (or do something || true)

E.g.:

+ fabric-ca-client register -c /Users/jkneubuh/github.com/sapthasurendran/fabric-samples/hardware-security-module/scripts/../ca-client-config/fabric-ca-client-config.yaml --mspdir /Users/jkneubuh/github.com/sapthasurendran/fabric-samples/hardware-security-module/scripts/../crypto-material/hsm/rcaadmin --id.name HSMUser --id.secret HSMUser --id.type client --caname org1-ca --id.maxenrollments 0 -m vcap.me -u https://org1-ca.vcap.me:443 --tls.certfiles /Users/jkneubuh/github.com/sapthasurendran/fabric-samples/hardware-security-module/scripts/../../test-network-k8s/build/cas/org1-ca/tlsca-cert.pem
2022/07/13 12:47:45 [INFO] Configuration file location: /Users/jkneubuh/github.com/sapthasurendran/fabric-samples/hardware-security-module/ca-client-config/fabric-ca-client-config.yaml
2022/07/13 12:47:45 [INFO] TLS Enabled
2022/07/13 12:47:45 [INFO] TLS Enabled
Error: Response from server: Error Code: 74 - Identity 'HSMUser' is already registered

+ echo user probably already registered, continuing

[jkneubuh]

Hi @sapthasurendran and @bestbeforetoday

Thank you for the addition of the new HSM sample for Fabric.

Reviewing this PR was a pleasure, and I will look forward to seeing the community continue the adoption of HSM as a "it just works" foundation of Fabric.

cheers