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

Support custom paths for cli cert checks #31988

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,4 @@ install_manifest.txt
*.cbp
!/.copr/Makefile
!/.buildkite/Makefile
.vscode/launch.json
9 changes: 7 additions & 2 deletions client/go/internal/cli/cmd/cert.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ func doCertAdd(cli *CLI, overwriteCertificate bool, args []string) error {
if err != nil {
return err
}
if pkg.HasCertificate() && !overwriteCertificate {
defaultCertPath := []string{"security/clients.pem"}
if pkg.HasCertificate(defaultCertPath) && !overwriteCertificate {
return errHint(fmt.Errorf("application package '%s' already contains a certificate", pkg.Path), "Use -f flag to force overwriting")
}
return requireCertificate(true, false, cli, target, pkg)
Expand All @@ -182,7 +183,11 @@ func requireCertificate(force, ignoreZip bool, cli *CLI, target vespa.Target, pk
if force {
return copyCertificate(tlsOptions, cli, pkg)
}
if pkg.HasCertificate() {
servicesXML, err := readServicesXML(pkg)
if err != nil {
return fmt.Errorf("Error reading services.xml: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Error messages should be lower-case as this may be concatenated with other errors further up.

Copy link
Author

Choose a reason for hiding this comment

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

Ah good spot!

}
if pkg.HasCertificate(servicesXML.CertPaths()) {
if cli.isCI() {
return nil // A matching certificate is not required in CI environments
}
Expand Down
9 changes: 8 additions & 1 deletion client/go/internal/cli/cmd/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,15 @@ $ vespa deploy -t cloud -z perf.aws-us-east-1c`,
return err
}
var result vespa.PrepareResult
var certPaths []string
servicesXML, err := readServicesXML(pkg)
if err != nil {
certPaths = []string{}
} else {
certPaths = servicesXML.CertPaths()
}
err = cli.spinner(cli.Stderr, "Uploading application package...", func() error {
result, err = vespa.Deploy(opts)
result, err = vespa.Deploy(opts, certPaths)
return err
})
if err != nil {
Expand Down
7 changes: 6 additions & 1 deletion client/go/internal/cli/cmd/prod.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,12 @@ $ vespa prod deploy`,
AuthorEmail: options.authorEmail,
SourceURL: options.sourceURL,
}
build, err := vespa.Submit(deployment, submission)

servicesXML, err := readServicesXML(pkg)
if err != nil {
return fmt.Errorf("Error reading services.xml: %w", err)
}
build, err := vespa.Submit(deployment, submission, servicesXML.CertPaths())
if err != nil {
return fmt.Errorf("could not deploy application: %w", err)
} else {
Expand Down
10 changes: 10 additions & 0 deletions client/go/internal/cli/cmd/prod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ func TestProdInit(t *testing.T) {
containerFragment := `<container id="qrs" version="1.0">
<document-api></document-api>
<search></search>
<clients>
<client id="test" permissions="read">
<certificate file="security/clients.pem"></certificate>
</client>
</clients>
<nodes count="4"></nodes>
</container>`
assert.Contains(t, servicesXML, containerFragment)
Expand Down Expand Up @@ -111,6 +116,11 @@ func createApplication(t *testing.T, pkgDir string, java bool, skipTests bool) {
<container id="qrs" version="1.0">
<document-api/>
<search/>
<clients>
<client id="test" permissions="read">
<certificate file="security/clients.pem"/>
</client>
</clients>
<nodes count="2">
<resources vcpu="4" memory="8Gb" disk="100Gb"/>
</nodes>
Expand Down
17 changes: 16 additions & 1 deletion client/go/internal/vespa/application.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,22 @@ type ApplicationPackage struct {
TestPath string
}

func (ap *ApplicationPackage) HasCertificate() bool { return ap.hasFile("security", "clients.pem") }
func (ap *ApplicationPackage) HasCertificate(certPaths []string) bool {
if len(certPaths) == 0 {
if ap.hasFile("security", "clients.pem") {
return true
} else {
return false
}
} else {
for _, certPath := range certPaths {
if ap.hasFile(certPath) {
return true
}
}
Comment on lines +31 to +35
Copy link
Member

Choose a reason for hiding this comment

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

This should check that all files are present, not just one.

Copy link
Author

Choose a reason for hiding this comment

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

👍

return false
}
}

func (ap *ApplicationPackage) HasMatchingCertificate(certificatePEM []byte) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to consider all certificates now, not just clients.pem. I think ApplicationPackage could have a method that reads the certificate paths from services XML, then both HasCertificate and HasMatchingCertificate can use this method and you won't have to pass around certPaths everywhere.

Copy link
Author

@olaughter olaughter Sep 7, 2024

Choose a reason for hiding this comment

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

Thanks @mpolden, I'll look at refactoring for this. The issue I had originally was that the xml/config.go file imports the vespa package, leading to a circular import if vespa/application.go wants to make use of the file parsing from xml/config.go. I'll revisit and see what I can do to make it work

Copy link
Author

@olaughter olaughter Sep 7, 2024

Choose a reason for hiding this comment

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

Hey, I've been playing around with this and think I could do with some steer on solving it.

I've been trying to get around the circular imports by changing the vespa package. xml/config.go imports System, from vespa, but moving that to its own package ended up with a lot of files changed without solving the issue. I also tried extracting ApplicationPackage, but again couldn't get that to work on its own. There might be a way of solving it this way but I don't feel confident enough to do it as it seems it would take a more dramatic refactoring then I'm comfortable with.

The only other option I could think of was to recreate some of the xml parsing functionality within ApplicationPackage. It wouldn't be very dry, but would enable this. What do you think is the best way forward?

(btw I'm on the vespa slack so let me know if its easier to chat this through there)

clientsPEM, err := os.ReadFile(filepath.Join(ap.Path, "security", "clients.pem"))
Expand Down
12 changes: 6 additions & 6 deletions client/go/internal/vespa/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -334,13 +334,13 @@ func Deactivate(deployment DeploymentOptions) error {
}

// Deploy deploys an application.
func Deploy(deployment DeploymentOptions) (PrepareResult, error) {
func Deploy(deployment DeploymentOptions, certPaths []string) (PrepareResult, error) {
var (
u *url.URL
err error
)
if deployment.Target.IsCloud() {
if err := checkDeploymentOpts(deployment); err != nil {
if err := checkDeploymentOpts(deployment, certPaths); err != nil {
return PrepareResult{}, err
}
if deployment.Target.Deployment().Zone.Environment == "" || deployment.Target.Deployment().Zone.Region == "" {
Expand Down Expand Up @@ -373,11 +373,11 @@ func copyToPart(dst *multipart.Writer, src io.Reader, fieldname, filename string
return nil
}

func Submit(opts DeploymentOptions, submission Submission) (int64, error) {
func Submit(opts DeploymentOptions, submission Submission, certPaths []string) (int64, error) {
if !opts.Target.IsCloud() {
return 0, fmt.Errorf("%s: deploy is unsupported by %s target", opts, opts.Target.Type())
}
if err := checkDeploymentOpts(opts); err != nil {
if err := checkDeploymentOpts(opts, certPaths); err != nil {
return 0, err
}
submitURL := opts.Target.Deployment().System.SubmitURL(opts.Target.Deployment())
Expand Down Expand Up @@ -449,8 +449,8 @@ func deployServiceDo(request *http.Request, timeout time.Duration, opts Deployme
return s.Do(request, timeout)
}

func checkDeploymentOpts(opts DeploymentOptions) error {
if opts.Target.Type() == TargetCloud && !opts.ApplicationPackage.HasCertificate() {
func checkDeploymentOpts(opts DeploymentOptions, certPaths []string) error {
if opts.Target.Type() == TargetCloud && !opts.ApplicationPackage.HasCertificate(certPaths) {
return fmt.Errorf("%s: missing certificate in package", opts)
}
if !opts.Target.IsCloud() && !opts.Version.IsZero() {
Expand Down
10 changes: 5 additions & 5 deletions client/go/internal/vespa/deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestDeploy(t *testing.T) {
Target: target,
ApplicationPackage: ApplicationPackage{Path: appDir},
}
_, err := Deploy(opts)
_, err := Deploy(opts, []string{})
assert.Nil(t, err)
assert.Equal(t, 1, len(httpClient.Requests))
req := httpClient.LastRequest
Expand All @@ -49,7 +49,7 @@ func TestDeployCloud(t *testing.T) {
Target: target,
ApplicationPackage: ApplicationPackage{Path: appDir},
}
_, err := Deploy(opts)
_, err := Deploy(opts, []string{})
require.Nil(t, err)
assert.Equal(t, 1, len(httpClient.Requests))
req := httpClient.LastRequest
Expand All @@ -62,7 +62,7 @@ func TestDeployCloud(t *testing.T) {
assert.False(t, hasDeployOptions)

opts.Version = version.MustParse("1.2.3")
_, err = Deploy(opts)
_, err = Deploy(opts, []string{})
require.Nil(t, err)
req = httpClient.LastRequest
values = parseMultiPart(t, req)
Expand All @@ -83,7 +83,7 @@ func TestSubmit(t *testing.T) {
ApplicationPackage: ApplicationPackage{Path: appDir},
}
httpClient.NextResponseString(200, `{"build": 42}`)
build, err := Submit(opts, Submission{})
build, err := Submit(opts, Submission{}, []string{})
require.Nil(t, err)
require.Equal(t, int64(42), build)
require.Nil(t, httpClient.LastRequest.ParseMultipartForm(1<<20))
Expand All @@ -102,7 +102,7 @@ func TestSubmit(t *testing.T) {
Description: "broken garbage",
AuthorEmail: "[email protected]",
SourceURL: "https://github.com/foo/repo",
})
}, []string{})
require.Nil(t, err)
require.Equal(t, int64(43), build)
require.Nil(t, httpClient.LastRequest.ParseMultipartForm(1<<20))
Expand Down
31 changes: 28 additions & 3 deletions client/go/internal/vespa/xml/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,30 @@ type Services struct {
}

type Container struct {
Root xml.Name `xml:"container"`
ID string `xml:"id,attr"`
Nodes Nodes `xml:"nodes"`
Root xml.Name `xml:"container"`
ID string `xml:"id,attr"`
Nodes Nodes `xml:"nodes"`
Clients Clients `xml:"clients"`
}

type Content struct {
ID string `xml:"id,attr"`
Nodes Nodes `xml:"nodes"`
}

type Clients struct {
Client []Client `xml:"client"`
}

type Client struct {
Id string `xml:"id,attr"`
Certificate Certificate `xml:"certificate"`
}

type Certificate struct {
File string `xml:"file,attr"`
}

type Nodes struct {
Count string `xml:"count,attr"`
Resources *Resources `xml:"resources,omitempty"`
Expand All @@ -98,6 +112,17 @@ type Resources struct {

func (s Services) String() string { return s.rawXML.String() }

// Reads file paths from services.xml
func (s Services) CertPaths() []string {
var certificates []string
for _, container := range s.Container {
for _, client := range container.Clients.Client {
certificates = append(certificates, client.Certificate.File)
}
}
return certificates
}

// Replace replaces any elements of name found under parentName with data.
func (s *Services) Replace(parentName, name string, data interface{}) error {
rewritten, err := Replace(&s.rawXML, parentName, name, data)
Expand Down
45 changes: 45 additions & 0 deletions client/go/internal/vespa/xml/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,51 @@ func TestReadServicesNoResources(t *testing.T) {
}
}

func TestReadClientPaths(t *testing.T) {
s := `
<services xmlns:deploy="vespa" xmlns:preprocess="properties">
<container id="qrs">
<clients>
<client id="test" permissions="read">
<certificate file="security/test.pem"/>
</client>
<client id="other" permissions="read">
<certificate file="security/other.pem"/>
</client>
</clients>
</container>
</services>
`
services, err := ReadServices(strings.NewReader(s))
if err != nil {
t.Fatal(err)
}
certPaths := services.CertPaths()
if got := certPaths[0]; got != "security/test.pem" {
t.Errorf("got %+v, want security/test.pem", got)
}
if got := certPaths[1]; got != "security/other.pem" {
t.Errorf("got %+v, want security/other.pem", got)
}
}

func TestReadNoClientPaths(t *testing.T) {
s := `
<services xmlns:deploy="vespa" xmlns:preprocess="properties">
<container id="qrs">
</container>
</services>
`
services, err := ReadServices(strings.NewReader(s))
if err != nil {
t.Fatal(err)
}
certPaths := services.CertPaths()
if got := certPaths; got != nil {
t.Errorf("got %+v, want nil", got)
}
}

func TestParseResources(t *testing.T) {
assertResources(t, "foo", Resources{}, true)
assertResources(t, "vcpu=2,memory=4Gb", Resources{}, true)
Expand Down