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.

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

Add SSL cert for accessing stored request API #1087

Merged
merged 8 commits into from
Nov 5, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
3 changes: 3 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ type Configuration struct {
BlacklistedAcctMap map[string]bool
// Is publisher/account ID required to be submitted in the OpenRTB2 request
AccountRequired bool `mapstructure:"account_required"`
// Local private file containing SSL certificates
PemCertsFile string `mapstructure:"certificates_file"`
}

const MIN_COOKIE_SIZE_BYTES = 500
Expand Down Expand Up @@ -704,6 +706,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("blacklisted_apps", []string{""})
v.SetDefault("blacklisted_accts", []string{""})
v.SetDefault("account_required", false)
v.SetDefault("certificates_file", "")

// Set environment variable support:
v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
Expand Down
3 changes: 3 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func TestDefaults(t *testing.T) {
cmpBools(t, "account_required", cfg.AccountRequired, false)
cmpInts(t, "metrics.influxdb.collection_rate_seconds", cfg.Metrics.Influxdb.MetricSendInterval, 20)
cmpBools(t, "account_adapter_details", cfg.Metrics.Disabled.AccountAdapterDetails, false)
cmpStrings(t, "certificates_file", cfg.PemCertsFile, "")
}

var fullConfig = []byte(`
Expand Down Expand Up @@ -105,6 +106,7 @@ adapters:
usersync_url: https://tag.adkernel.com/syncr?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&r=
blacklisted_apps: ["spamAppID","sketchy-app-id"]
account_required: true
certificates_file: /etc/ssl/cert.pem
`)

var adapterExtraInfoConfig = []byte(`
Expand Down Expand Up @@ -266,6 +268,7 @@ func TestFullConfig(t *testing.T) {
cmpStrings(t, "adapters.rhythmone.usersync_url", cfg.Adapters[string(openrtb_ext.BidderRhythmone)].UserSyncURL, "https://sync.1rx.io/usersync2/rmphb?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&redir=http%3A%2F%2Fprebid-server.prebid.org%2F%2Fsetuid%3Fbidder%3Drhythmone%26gdpr%3D{{.GDPR}}%26gdpr_consent%3D{{.GDPRConsent}}%26uid%3D%5BRX_UUID%5D")
cmpBools(t, "account_required", cfg.AccountRequired, true)
cmpBools(t, "account_adapter_details", cfg.Metrics.Disabled.AccountAdapterDetails, true)
cmpStrings(t, "certificates_file", cfg.PemCertsFile, "/etc/ssl/cert.pem")
}

func TestUnmarshalAdapterExtraInfo(t *testing.T) {
Expand Down
12 changes: 11 additions & 1 deletion router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,22 @@ func New(cfg *config.Configuration, rateConvertor *currencies.RateConverter) (r
r = &Router{
Router: httprouter.New(),
}

// For bid processing, we need both the hardcoded certificates and the certificates found in container's
// local file system
certPool := ssl.GetRootCAPool()
var readCertErr Error
certPool, readCertErr = ssl.AppendPEMFileToRootCAPool(certPool, cfg.PemCertsFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a check if the string is empty?

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 AppendPEMFileToRootCAPool(certPool *x509.CertPool, pemFileName string) function checks for an empty string but I removed the error it was returning when the string is empty because as of right now, we have no certificates file and it seems odd to return an error if the certificates file has not been defined in the configuration.

Do you prefer the calling function to do the empty string check?

if readCertErr != nil {
glog.Infof("Could not read certificates file: %s \n", readCertErr.Error())
}

theClient := &http.Client{
Transport: &http.Transport{
MaxIdleConns: cfg.Client.MaxIdleConns,
MaxIdleConnsPerHost: cfg.Client.MaxIdleConnsPerHost,
IdleConnTimeout: time.Duration(cfg.Client.IdleConnTimeout) * time.Second,
TLSClientConfig: &tls.Config{RootCAs: ssl.GetRootCAPool()},
TLSClientConfig: &tls.Config{RootCAs: certPool},
},
}
// Hack because of how legacy handles districtm
Expand Down
11 changes: 11 additions & 0 deletions ssl/mockcertificates/mock-certs.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-----BEGIN CERTIFICATE-----
MIIBhTCCASugAwIBAgIQIRi6zePL6mKjOipn+dNuaTAKBggqhkjOPQQDAjASMRAw
DgYDVQQKEwdBY21lIENvMB4XDTE3MTAyMDE5NDMwNloXDTE4MTAyMDE5NDMwNlow
EjEQMA4GA1UEChMHQWNtZSBDbzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABD0d
7VNhbWvZLWPuj/RtHFjvtJBEwOkhbN/BnnE8rnZR8+sbwnc/KhCk3FhnpHZnQz7B
5aETbbIgmuvewdjvSBSjYzBhMA4GA1UdDwEB/wQEAwICpDATBgNVHSUEDDAKBggr
BgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MCkGA1UdEQQiMCCCDmxvY2FsaG9zdDo1
NDUzgg4xMjcuMC4wLjE6NTQ1MzAKBggqhkjOPQQDAgNIADBFAiEA2zpJEPQyz6/l
Wf86aX6PepsntZv2GYlA5UpabfT2EZICICpJ5h/iI+i341gBmLiAFQOyTDT+/wQc
6MF9+Yw1Yy0t
-----END CERTIFICATE-----
24 changes: 24 additions & 0 deletions ssl/ssl.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package ssl

import (
"crypto/x509"
"fmt"
"io/ioutil"
)

// from https://medium.com/@kelseyhightower/optimizing-docker-images-for-static-binaries-b5696e26eb07
Expand All @@ -16,6 +18,28 @@ func GetRootCAPool() *x509.CertPool {
return pool
}

// Appends certificates to the `x509.CertPool` from a `.pem` private local file. On many Linux
// systems, /etc/ssl/cert.pem will contain the system wide set but in our case, we'll pull
// the certificate file path from the `Configuration` struct
func AppendPEMFileToRootCAPool(certPool *x509.CertPool, pemFileName string) (*x509.CertPool, error) {
if certPool == nil {
certPool = x509.NewCertPool()
}
if pemFileName != "" {
//read file and place it's contents in `pemCerts`
pemCerts, err := ioutil.ReadFile(pemFileName)
if err != nil {
return certPool, fmt.Errorf("Failed to read file %s: %v", pemFileName, err)
}

//`pemCerts` has been obtained, append to certPool
certPool.AppendCertsFromPEM(pemCerts)
} else {
return certPool, fmt.Errorf("Path to local PEM file containing SSL certificates is\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error message is confusing ... is it really an error to fail to read a cert file when none was given?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

}
return certPool, nil
}

var pemCerts = []byte(`
-----BEGIN CERTIFICATE-----
MIIDzzCCAregAwIBAgIDAWweMA0GCSqGSIb3DQEBBQUAMIGNMQswCQYDVQQGEwJB
Expand Down
54 changes: 54 additions & 0 deletions ssl/ssl_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package ssl

import (
"crypto/x509"
"testing"

"github.com/stretchr/testify/assert"
)

func TestCertsFromFilePoolExists(t *testing.T) {
// Load hardcoded certificates found in ssl.go
certPool := GetRootCAPool()

// Assert loaded certificates by looking at the lenght of the subjects array of strings
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: lenght

subjects := certPool.Subjects()
hardCodedSubNum := len(subjects)
assert.True(t, hardCodedSubNum > 0)

// Load certificates from file
certificatesFile := "mockcertificates/mock-certs.pem"
certPool, err := AppendPEMFileToRootCAPool(certPool, certificatesFile)

// Assert loaded certificates by looking at the lenght of the subjects array of strings
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: lenght

assert.NoError(t, err, "Error thrown by AppendPEMFileToRootCAPool while loading file %s: %v", certificatesFile, err)
subjects = certPool.Subjects()
subNumIncludingFile := len(subjects)
assert.True(t, subNumIncludingFile > hardCodedSubNum, "subNumIncludingFile should be greater than hardCodedSubNum")
}

func TestCertsFromFilePoolDontExist(t *testing.T) {
// Empty certpool
var certPool *x509.CertPool = nil

// Load certificates from file
certificatesFile := "mockcertificates/mock-certs.pem"
certPool, err := AppendPEMFileToRootCAPool(certPool, certificatesFile)

// Assert loaded certificates by looking at the lenght of the subjects array of strings
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: lenght

assert.NoError(t, err, "Error thrown by AppendPEMFileToRootCAPool while loading file %s: %v", certificatesFile, err)
subjects := certPool.Subjects()
assert.Equal(t, len(subjects), 1, "We only loaded one vertificate from the file, len(subjects) should equal 1")
}

func TestAppendPEMFileToRootCAPoolFail(t *testing.T) {
// Empty certpool
var certPool *x509.CertPool

// Load certificates from file
fakeCertificatesFile := "mockcertificates/NO-FILE.pem"
certPool, err := AppendPEMFileToRootCAPool(certPool, fakeCertificatesFile)

// Assert loaded certificates by looking at the lenght of the subjects array of strings
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is likely a copy/paste from the other tests, but doesn't make sense in this instance because an error is expected and the length is never checked.

assert.Errorf(t, err, "AppendPEMFileToRootCAPool should throw an error by while loading fake file %s \n", fakeCertificatesFile)
}