Skip to content

Commit

Permalink
[FAB-4017] Duplicate flags registered for 'Hosts'
Browse files Browse the repository at this point in the history 
Currently there are two places you can specify 'hosts' when
performing an enrollment. One place is to specify it as part
of the CSR and the second place is in the Enrollment
Request API. For example, you see duplicate hosts flags on
client side:

--csr.hosts stringSlice A list of space-separated host names in a
certificate signing request

--enrollment.hosts string Comma-separated host list

It is unclear to a user as to which one to use or which one will
take precedence. Currently the hosts that is part of the Enrollment
API will only take affect and the 'hosts' from the CSR will get
ignored. This change removes 'hosts' from Enrollment
API, and uses the 'hosts' defined in the CSR.

See [FAB-4017] for more info.

Adds test-cases to get util package test coverage to over 85%

Change-Id: I1d910b29cd9aea98db66511da39345e71df9a525
Signed-off-by: Saad Karim <[email protected]>
  • Loading branch information
Saad Karim committed May 24, 2017
1 parent 4fa7ec0 commit 99fd112
Show file tree
Hide file tree
Showing 12 changed files with 111 additions and 73 deletions.
4 changes: 0 additions & 4 deletions api/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ type EnrollmentRequest struct {
Name string `json:"name" skip:"true"`
// The secret returned via Register
Secret string `json:"secret,omitempty" skip:"true"`
// Hosts is a comma-separated host list in the CSR
Hosts string `json:"hosts,omitempty" help:"Comma-separated host list"`
// Profile is the name of the signing profile to use in issuing the certificate
Profile string `json:"profile,omitempty" help:"Name of the signing profile to use in issuing the certificate"`
// Label is the label to use in HSM operations
Expand All @@ -75,8 +73,6 @@ type EnrollmentRequest struct {
// ReenrollmentRequest is a request to reenroll an identity.
// This is useful to renew a certificate before it has expired.
type ReenrollmentRequest struct {
// Hosts is a comma-separated host list in the CSR
Hosts string `json:"hosts,omitempty"`
// Profile is the name of the signing profile to use in issuing the certificate
Profile string `json:"profile,omitempty"`
// Label is the label to use in HSM operations
Expand Down
32 changes: 26 additions & 6 deletions cmd/fabric-ca-client/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ import (
"path/filepath"
"strings"
"testing"
"time"

"github.com/cloudflare/cfssl/config"
"github.com/hyperledger/fabric-ca/api"
"github.com/hyperledger/fabric-ca/lib"
"github.com/hyperledger/fabric-ca/lib/dbutil"
Expand Down Expand Up @@ -289,7 +291,7 @@ func testEnroll(t *testing.T) {
t.Errorf("No username/password provided, should have errored")
}

err = RunMain([]string{cmdName, "enroll", "-u", "http://admin:adminpw@localhost:7054"})
err = RunMain([]string{cmdName, "enroll", "-u", "http://admin:adminpw@localhost:7054", "-M", filepath.Join(filepath.Dir(defYaml), "msp")})
if err != nil {
t.Errorf("client enroll -u failed: %s", err)
}
Expand Down Expand Up @@ -337,11 +339,15 @@ func testReenroll(t *testing.T) {
t.Log("Testing Reenroll CMD")
defYaml = util.GetDefaultConfigFile("fabric-ca-client")

err := RunMain([]string{cmdName, "reenroll", "-u", "http://localhost:7054", "--enrollment.hosts", "host1,host2"})
err := RunMain([]string{cmdName, "reenroll", "-u", "http://localhost:7054", "--csr.hosts", "host1"})
if err != nil {
t.Errorf("client reenroll --url -f failed: %s", err)
}

err = util.CheckHostsInCert(filepath.Join(filepath.Dir(defYaml), "msp", "signcerts", "cert.pem"), "host1")
if err != nil {
t.Error(err)
}
os.Remove(defYaml)
}

Expand Down Expand Up @@ -509,10 +515,10 @@ func testProfiling(t *testing.T) {
mProfExpected bool
cProfExpected bool
}{
{"heap", []string{cmdName, "reenroll", "-u", "http://localhost:7054", "--enrollment.hosts", "host1,host2"}, true, false},
{"cpu", []string{cmdName, "reenroll", "-u", "http://localhost:7054", "--enrollment.hosts", "host1,host2"}, false, true},
{"", []string{cmdName, "reenroll", "-u", "http://localhost:7054", "--enrollment.hosts", "host1,host2"}, false, false},
{"xxx", []string{cmdName, "reenroll", "-u", "http://localhost:7054", "--enrollment.hosts", "host1,host2"}, false, false},
{"heap", []string{cmdName, "reenroll", "-u", "http://localhost:7054"}, true, false},
{"cpu", []string{cmdName, "reenroll", "-u", "http://localhost:7054"}, false, true},
{"", []string{cmdName, "reenroll", "-u", "http://localhost:7054"}, false, false},
{"xxx", []string{cmdName, "reenroll", "-u", "http://localhost:7054"}, false, false},
}
wd, err := os.Getwd()
if err != nil {
Expand Down Expand Up @@ -794,6 +800,16 @@ func getCAConfig() *lib.CAConfig {
"org1": nil,
}

defaultSigningProfile := &config.SigningProfile{
Usage: []string{"cert sign"},
ExpiryString: "8000h",
Expiry: time.Hour * 8000,
}

profiles := map[string]*config.SigningProfile{
"ca": defaultSigningProfile,
}

return &lib.CAConfig{
CA: lib.CAInfo{
Keyfile: keyfile,
Expand All @@ -803,6 +819,10 @@ func getCAConfig() *lib.CAConfig {
CSR: api.CSRInfo{
CN: "TestCN",
},
Signing: &config.Signing{
Default: defaultSigningProfile,
Profiles: profiles,
},
}
}

Expand Down
1 change: 0 additions & 1 deletion cmd/fabric-ca-client/reenroll.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ func runReenroll() error {
}

req := &api.ReenrollmentRequest{
Hosts: clientCfg.Enrollment.Hosts,
Label: clientCfg.Enrollment.Label,
Profile: clientCfg.Enrollment.Profile,
CSR: &clientCfg.CSR,
Expand Down
10 changes: 5 additions & 5 deletions docs/source/users-guide.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ The following shows the Fabric CA server usage message.
--db.tls.enabled Enable TLS for client connection
--db.type string Type of database; one of: sqlite3, postgres, mysql (default "sqlite3")
-d, --debug Enable debug level logging
--intermediate.enrollment.hosts string Comma-separated host list
--intermediate.enrollment.label string Label to use in HSM operations
--intermediate.enrollment.profile string Name of the signing profile to use in issuing the certificate
--intermediate.parentserver.caname string Name of the CA to connect to on fabric-ca-serve
Expand Down Expand Up @@ -224,7 +223,6 @@ The following shows the Fabric CA client usage message:
--csr.hosts stringSlice A list of space-separated host names in a certificate signing request
--csr.serialnumber string The serial number in a certificate signing request
-d, --debug Enable debug level logging
--enrollment.hosts string Comma-separated host list
--enrollment.label string Label to use in HSM operations
--enrollment.profile string Name of the signing profile to use in issuing the certificate
--id.affiliation string The identity's affiliation
Expand All @@ -242,7 +240,11 @@ The following shows the Fabric CA client usage message:

Use "fabric-ca-client [command] --help" for more information about a command.

Note that command line options that are string slices (lists) can be specified either by specifying the option with space-separated list elements or by specifying the option multiple times, each with a string value that make up the list. For example, to specify ``host1`` and ``host2`` for `csr.hosts` option, you can either pass `--csr.hosts "host1 host2"` or `--csr.hosts host1 --csr.hosts host2`
Note that command line options that are string slices (lists) can be specified either
by specifying the option with space-separated list elements or by specifying the option
multiple times, each with a string value that make up the list. For example, to specify
``host1`` and ``host2`` for `csr.hosts` option, you can either pass `--csr.hosts
"host1 host2"` or `--csr.hosts host1 --csr.hosts host2`

`Back to Top`_

Expand Down Expand Up @@ -481,7 +483,6 @@ the server's home directory (see `Fabric CA Server <#server>`__ section more inf
caname:

enrollment:
hosts:
profile:
label:

Expand Down Expand Up @@ -559,7 +560,6 @@ the client's home directory (see `Fabric CA Client <#client>`__ section more inf
# Enrollment section used to enroll an identity with Fabric CA server
#############################################################################
enrollment:
hosts:
profile:
label:

Expand Down
5 changes: 3 additions & 2 deletions lib/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
cfsslapi "github.com/cloudflare/cfssl/api"
"github.com/cloudflare/cfssl/csr"
"github.com/cloudflare/cfssl/log"
"github.com/cloudflare/cfssl/signer"
"github.com/hyperledger/fabric-ca/api"
"github.com/hyperledger/fabric-ca/lib/tls"
"github.com/hyperledger/fabric-ca/util"
Expand Down Expand Up @@ -172,7 +171,9 @@ func (c *Client) Enroll(req *api.EnrollmentRequest) (*EnrollmentResponse, error)
CAName: req.CAName,
}

reqNet.SignRequest.Hosts = signer.SplitHosts(req.Hosts)
if req.CSR != nil {
reqNet.SignRequest.Hosts = req.CSR.Hosts
}
reqNet.SignRequest.Request = string(csrPEM)
reqNet.SignRequest.Profile = req.Profile
reqNet.SignRequest.Label = req.Label
Expand Down
5 changes: 3 additions & 2 deletions lib/identity.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"net/http"

"github.com/cloudflare/cfssl/log"
"github.com/cloudflare/cfssl/signer"
"github.com/hyperledger/fabric-ca/api"
"github.com/hyperledger/fabric-ca/util"
"github.com/hyperledger/fabric/bccsp"
Expand Down Expand Up @@ -117,7 +116,9 @@ func (i *Identity) Reenroll(req *api.ReenrollmentRequest) (*EnrollmentResponse,
}

// Get the body of the request
reqNet.SignRequest.Hosts = signer.SplitHosts(req.Hosts)
if req.CSR != nil {
reqNet.SignRequest.Hosts = req.CSR.Hosts
}
reqNet.SignRequest.Request = string(csrPEM)
reqNet.SignRequest.Profile = req.Profile
reqNet.SignRequest.Label = req.Label
Expand Down
28 changes: 2 additions & 26 deletions lib/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package lib_test

import (
"bytes"
"encoding/asn1"
"fmt"
"io/ioutil"
"net/http"
Expand Down Expand Up @@ -314,7 +313,7 @@ func TestIntermediateServerWithTLS(t *testing.T) {
}

// Check that CSR fields are correctly getting inserted into certificate
err = checkHostsInCert(filepath.Join(intermediateDir, "ca-cert.pem"), "testhost")
err = util.CheckHostsInCert(filepath.Join(intermediateDir, "ca-cert.pem"), "testhost")
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -603,7 +602,7 @@ func TestMultiCAWithIntermediate(t *testing.T) {
}

// Check that CSR fields are correctly getting inserted into certificate
err = checkHostsInCert(filepath.Join("../testdata/ca/intermediateca/ca1", "ca-cert.pem"), "testhost1")
err = util.CheckHostsInCert(filepath.Join("../testdata/ca/intermediateca/ca1", "ca-cert.pem"), "testhost1")
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -951,26 +950,3 @@ func getTLSConfig(srv *Server, clientAuthType string, clientRootCerts []string)

return srv
}

func checkHostsInCert(certFile string, host string) error {
certBytes, err := ioutil.ReadFile(certFile)
if err != nil {
return fmt.Errorf("Failed to read file: %s", err)
}

cert, err := util.GetX509CertificateFromPEM(certBytes)
if err != nil {
return fmt.Errorf("Failed to get certificate: %s", err)
}
// Run through the extensions for the certificates
for _, ext := range cert.Extensions {
// asn1 identifier for 'Subject Alternative Name'
if ext.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 17}) {
if !strings.Contains(string(ext.Value), host) {
return fmt.Errorf("Failed to correctly insert host '%s' into certificate", host)
}
}
}

return nil
}
3 changes: 2 additions & 1 deletion scripts/fvt/fabric-ca_utils
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ enroll() {
setTLS
$FABRIC_CA_CLIENTEXEC enroll -u "${PROTO}${username}:${userpswd}@${HOST}:${PORT}" $TLSOPT \
-c $ENROLLCONFIG \
--enrollment.hosts "$username@fab-client.raleigh.ibm.com,$username.fabric.raleigh.ibm.com,127.0.0.2"
--csr.hosts "$username@fab-client.raleigh.ibm.com" \
--csr.hosts "$username.fabric.raleigh.ibm.com,127.0.0.2"
RC=$?
if test -n "$FABRIC_CA_DEBUG"; then
$(test "$RC" -eq 0 && $($FABRIC_CA_DEBUG)) && printAuth $FABRIC_CA_CERT_FILE $FABRIC_CA_KEY_FILE
Expand Down
14 changes: 0 additions & 14 deletions swagger/swagger-fabric-ca.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,6 @@
"schema": {
"type": "object",
"properties": {
"hosts": {
"type": [
"string",
"null"
],
"description": "A comma-separated list of host names to associate with the certificate."
},
"request": {
"type": "string",
"description": "A PEM-encoded string containing the CSR (Certificate Signing Request) based on PKCS #10."
Expand Down Expand Up @@ -276,13 +269,6 @@
"schema": {
"type": "object",
"properties": {
"hosts": {
"type": [
"string",
"null"
],
"description": "A comma-separated list of host names to associate with the certificate."
},
"request": {
"type": "string",
"description": "A PEM-encoded string containing the CSR (Certificate Signing Request) based on PKCS #10."
Expand Down
20 changes: 11 additions & 9 deletions util/flag_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,17 @@ import (

// A test struct
type A struct {
ASlice []string `help:"Slice description"`
AStr string `def:"defval" help:"Str1 description"`
AInt int `def:"10" help:"Int1 description"`
AB B `help:"FB description"`
AStr2 string `skip:"true"`
AIntArray []int `help:"IntArray description"`
AMap map[string]string `skip:"true"`
ABPtr *B `help:"FBP description"`
AInterface interface{} `skip:"true"`
ASlice []string `help:"Slice description"`
AStr string `def:"defval" help:"Str1 description"`
AInt int `def:"10" help:"Int1 description"`
AB B `help:"FB description"`
AStr2 string `skip:"true"`
AIntArray []int `help:"IntArray description"`
AMap map[string]string `skip:"true"`
ABPtr *B `help:"FBP description"`
AInterface interface{} `skip:"true"`
aUnexported string
ABad ABad `skip:"true"`
}

// B test struct
Expand Down
31 changes: 31 additions & 0 deletions util/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"crypto/ecdsa"
"crypto/rsa"
"crypto/x509"
"encoding/asn1"
"encoding/base64"
"encoding/json"
"encoding/pem"
Expand Down Expand Up @@ -599,3 +600,33 @@ func NormalizeFileList(files []string, homeDir string) ([]string, error) {

return files, nil
}

// CheckHostsInCert checks to see if host correctly inserted into certificate
func CheckHostsInCert(certFile string, host string) error {
containsHost := false
certBytes, err := ioutil.ReadFile(certFile)
if err != nil {
return fmt.Errorf("Failed to read file: %s", err)
}

cert, err := GetX509CertificateFromPEM(certBytes)
if err != nil {
return fmt.Errorf("Failed to get certificate: %s", err)
}
// Run through the extensions for the certificates
for _, ext := range cert.Extensions {
// asn1 identifier for 'Subject Alternative Name'
if ext.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 17}) {
if !strings.Contains(string(ext.Value), host) {
return fmt.Errorf("Host '%s' was not found in the certificate in file '%s'", host, certFile)
}
containsHost = true
}
}

if !containsHost {
return errors.New("Certificate contains no hosts")
}

return nil
}
Loading

0 comments on commit 99fd112

Please sign in to comment.