Crash when using v1.Security.AddCertificate()

[bodgit]

Describe the bug I'm trying to use v1.Security.AddCertificate() as part of atlassian/terraform-provider-artifactory#38 and I'm either doing something wrong or there's a bug here.

To Reproduce My provider code looks like this:

func resourceCertificateUpdate(d *schema.ResourceData, m interface{}) error {
        c := m.(*artifactory.Artifactory)

        pem, err := os.Open(d.Get("path").(string))
        if err != nil {
                return err
        }
        defer pem.Close()

        _, _, err = c.V1.Security.AddCertificate(context.Background(), d.Id(), pem)
        if err != nil {
                return err
        }

        return resourceCertificateRead(d, m)
}

v1.Security.AddCertificate() wants the certificate passed as an *os.File hence I assume I just open the file and pass it in like this? The file is open successfully and if I read it, there's clearly PEM data there.

Expected behavior I would expect the call to succeed.

Log Output There is a lot of Terraform log, but the top-most bits are:

2019-05-09T16:28:39.942+0100 [DEBUG] plugin.terraform-provider-artifactory: [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xd0f3ca]
2019-05-09T16:28:39.942+0100 [DEBUG] plugin.terraform-provider-artifactory:
2019-05-09T16:28:39.942+0100 [DEBUG] plugin.terraform-provider-artifactory: goroutine 58 [running]:
2019-05-09T16:28:39.942+0100 [DEBUG] plugin.terraform-provider-artifactory: github.com/atlassian/go-artifactory/artifactory/transport.(*BasicAuth).RoundTrip(0xc00013d2c0, 0xc000458600, 0xc00013d2c0, 0x0, 0x0)
2019-05-09T16:28:39.942+0100 [DEBUG] plugin.terraform-provider-artifactory:     .../work/src/github.com/atlassian/go-artifactory/artifactory/transport/basicauth.go:44 +0x19a

It's crashing on this line: https://github.com/atlassian/go-artifactory/blob/dc978cf4e4520147647dfdee47340c88039225db/artifactory/transport/basicauth.go#L44

req.GetBody() is nil. I added some debug to print the request before it gets sent, and this is what it looks like:

&{POST https://artifactory.example.com/artifactory/api/system/security/certificates/test HTTP/1.1 1 1 map[Accept:[application/json] Content-Type:[text/plain] User-Agent:[go-artifactory]] 0xc0000ad2d0 <nil> 0 [] false artifactory.example.com map[] map[] <nil> map[]   <nil> <nil> <nil> <nil>}

GetBody() is the <nil> following the 0xc0000ad2d0 address following the http.Request struct.

Desktop (please complete the following information):

• OS: Linux, go version go1.12.4 linux/amd64

Additional context Incidentally, v1.Security.AddCertificate() calls client.NewClient() which wants an io.Reader instead of a *os.File; using the former everywhere instead would mean you could feed in a stream of bytes for the PEM from a string rather than always having to reference an existing file on the disk, assuming I've got the API right.

[bodgit]

If I make the following change:

diff --git a/artifactory/v1/security.go b/artifactory/v1/security.go
index cc8d749..9cf5b53 100644
--- a/artifactory/v1/security.go
+++ b/artifactory/v1/security.go
@@ -5,10 +5,11 @@ import (
        "context"
        "encoding/json"
        "fmt"
-       "github.com/atlassian/go-artifactory/v2/artifactory/client"
+       "io"
        "net/http"
-       "os"
        "strings"
+
+       "github.com/atlassian/go-artifactory/v2/artifactory/client"
 )

 type SecurityService Service
@@ -1035,7 +1036,7 @@ func (s *SecurityService) GetCertificates(ctx context.Context) (*[]CertificateDe
 // Adds an SSL certificate.
 // Since:5.4.0
 // Security: Requires an admin user
-func (s *SecurityService) AddCertificate(ctx context.Context, alias string, pem *os.File) (*client.Status, *http.Response, error) {
+func (s *SecurityService) AddCertificate(ctx context.Context, alias string, pem io.Reader) (*client.Status, *http.Response, error) {
        path := fmt.Sprintf("/api/system/security/certificates/%s", alias)
        req, err := s.client.NewRequest("POST", path, pem)
        if err != nil {

and then use strings.NewReader() with the PEM content like this in my Terraform resource:

func resourceCertificateUpdate(d *schema.ResourceData, m interface{}) error {
        c := m.(*artifactory.Artifactory)

        _, _, err := c.V1.Security.AddCertificate(context.Background(), d.Id(), strings.NewReader(d.Get("content").(string)))
        if err != nil {
                return err
        }

        return resourceCertificateRead(d, m)
}

it works fine. It's down to something to do with specifically passing a file. Any ideas?

[bodgit]

I've created PR #17 to change the signature of AddCertificate() as I think it's a useful change anyway and shouldn't break backwards compatibility given one is the interface of the other.

[dillon-giacoppo]

Awesome, thanks for finding and fixing this!

[bodgit]

Sorry, I think there's a misunderstanding here; there's still a crash if you pass an open file even with the change applied, just with that change in place the PEM data can at least be passed by some other means that doesn't explode.

[dillon-giacoppo]

Oh no worries, I'll check this out hopefully early next week.