Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use receptorNames (oid=otherNames) in tls bootstrap; fixed associated unit tests for receptor and receptorctl #578

Merged
merged 3 commits into from
Apr 1, 2022
Merged

use receptorNames (oid=otherNames) in tls bootstrap; fixed associated unit tests for receptor and receptorctl #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b2450f
use receptorNames (oid=otherNames) in tls-server bootstrap; fixed ass…
Mar 15, 2022
58bc5ca
refactored oid check to a function; consuming said function in both s…
Mar 21, 2022
0466af0
added utils/common.go code to provide a common utils package; reworde…
Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions pkg/netceptor/netceptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,21 +1081,10 @@ func ReceptorVerifyFunc(tlscfg *tls.Config, pinnedFingerprints [][]byte, expecte
}

if expectedHostnameType == ExpectedHostnameTypeReceptor {
var receptorNames []string
receptorNames, err = utils.ReceptorNames(certs[0].Extensions)
found, receptorNames, err := utils.ParseReceptorNamesFromCert(certs[0], expectedHostname)
if err != nil {
logger.Error("RVF failed to get ReceptorNames: %s", err)

return err
}
found := false
for _, receptorName := range receptorNames {
if receptorName == expectedHostname {
found = true

break
}
}
if !found {
logger.Error("RVF ReceptorNameError: expected %s but found %s", expectedHostname, strings.Join(receptorNames, ", "))

Expand Down
69 changes: 55 additions & 14 deletions pkg/netceptor/tlsconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import (
"crypto/tls"
"crypto/x509"
"encoding/hex"
"encoding/pem"
"fmt"
"io/ioutil"
"strings"

"github.com/ansible/receptor/pkg/utils"
"github.com/ghjm/cmdline"
)

Expand Down Expand Up @@ -39,14 +41,39 @@ func decodeFingerprints(fingerprints []string) ([][]byte, error) {
return fingerprintBytes, nil
}

func checkCertificatesMatchNodeID(certbytes []byte, n *Netceptor, certName string, certPath string) error {
block, _ := pem.Decode(certbytes)

if block == nil {
return fmt.Errorf("failed to parse certfifcate PEM")
}

parsedCert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
return err
}

found, receptorNames, err := utils.ParseReceptorNamesFromCert(parsedCert, n.nodeID)
if err != nil {
return err
}

if !found {
Copy link
Member

Choose a reason for hiding this comment

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

we still need to return without error here if len(receptorNames) == 0

if I create a cert without a nodeID, and use that on a config with node ID foo, but I see this error

[sbf@fedora sockceptor]$ ./receptor -c foo.yml
Error: MainInstance.nodeID=foo not found in certificate name(s); names found=[]; cfg section=server2; server cert=/home/sbf/sockceptor/certs/bob.crt

we want this to be permissible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fosterseth - just thinking about this before i write a patch. would it be more optimal for the consumer to just flip the bit for the SkipReceptorNamesCheck flag to true if they give a ReceptorName/OID="" and skip the check altogether?

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with that option

fosterseth marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("MainInstance.nodeID=%s not found in certificate name(s); names found=%s; cfg section=%s; server cert=%s", MainInstance.nodeID, fmt.Sprint(receptorNames), certName, certPath)
}

return nil
}

// tlsServerCfg stores the configuration options for a TLS server.
type tlsServerCfg struct {
Name string `required:"true" description:"Name of this TLS server configuration"`
Cert string `required:"true" description:"Server certificate filename"`
Key string `required:"true" description:"Server private key filename"`
RequireClientCert bool `description:"Require client certificates" default:"false"`
ClientCAs string `description:"Filename of CA bundle to verify client certs with"`
PinnedClientCert []string `description:"Pinned fingerprint of required client certificate"`
Name string `required:"true" description:"Name of this TLS server configuration"`
Cert string `required:"true" description:"Server certificate filename"`
Key string `required:"true" description:"Server private key filename"`
RequireClientCert bool `required:"false" description:"Require client certificates" default:"false"`
ClientCAs string `required:"false" description:"Filename of CA bundle to verify client certs with"`
PinnedClientCert []string `required:"false" description:"Pinned fingerprint of required client certificate"`
SkipReceptorNamesCheck bool `required:"false" description:"Skip verifying ReceptorNames OIDs in certificate at startup" default:"false"`
}

// Prepare creates the tls.config and stores it in the global map.
Expand All @@ -56,18 +83,24 @@ func (cfg tlsServerCfg) Prepare() error {
MinVersion: tls.VersionTLS12,
}

certbytes, err := ioutil.ReadFile(cfg.Cert)
certBytes, err := ioutil.ReadFile(cfg.Cert)
if err != nil {
return err
}
keybytes, err := ioutil.ReadFile(cfg.Key)
if err != nil {
return err
}
cert, err := tls.X509KeyPair(certbytes, keybytes)
cert, err := tls.X509KeyPair(certBytes, keybytes)
if err != nil {
return err
}
// check server crt to ensure that the receptor NodeID is in the client certificate as an OID
if !cfg.SkipReceptorNamesCheck {
if err := checkCertificatesMatchNodeID(certBytes, MainInstance, cfg.Name, cfg.Cert); err != nil {
return err
}
}

tlscfg.Certificates = []tls.Certificate{cert}

Expand Down Expand Up @@ -106,12 +139,13 @@ func (cfg tlsServerCfg) Prepare() error {

// tlsClientConfig stores the configuration options for a TLS client.
type tlsClientConfig struct {
Name string `required:"true" description:"Name of this TLS client configuration"`
Cert string `required:"false" description:"Client certificate filename"`
Key string `required:"false" description:"Client private key filename"`
RootCAs string `required:"false" description:"Root CA bundle to use instead of system trust"`
InsecureSkipVerify bool `required:"false" description:"Accept any server cert" default:"false"`
PinnedServerCert []string `required:"false" description:"Pinned fingerprint of required server certificate"`
Name string `required:"true" description:"Name of this TLS client configuration"`
Cert string `required:"false" description:"Client certificate filename"`
Key string `required:"false" description:"Client private key filename"`
RootCAs string `required:"false" description:"Root CA bundle to use instead of system trust"`
InsecureSkipVerify bool `required:"false" description:"Accept any server cert" default:"false"`
PinnedServerCert []string `required:"false" description:"Pinned fingerprint of required server certificate"`
SkipReceptorNamesCheck bool `required:"false" description:"if enabled, skip verifying ReceptorNames OIDs in certificate at startup"`
Copy link
Member

Choose a reason for hiding this comment

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

golang will default this to false, but for consistency and clarify I think we can add default:"false" , similar to InsecureSkipVerify

To make the description a little shorter, we could omit "if enabled," and just start with "Skip.."

}

// Prepare creates the tls.config and stores it in the global map.
Expand All @@ -137,6 +171,13 @@ func (cfg tlsClientConfig) Prepare() error {
if err != nil {
return err
}

// check client crt to ensure that the receptor NodeID is in the client certificate as an OID
if !cfg.SkipReceptorNamesCheck {
if err := checkCertificatesMatchNodeID(certBytes, MainInstance, cfg.Name, cfg.Cert); err != nil {
return err
}
}
tlscfg.Certificates = []tls.Certificate{cert}
}

Expand Down
Loading