[specific ci=Group23-VIC-Machine-Service] Accept session clone ticket via request header #6587

jzt · 2017-10-18T21:48:42Z

This change allows the vic-machine-server to accept a vSphere session clone ticket in the X-VMWARE-TICKET header of incoming requests. This ticket is used by govmomi to clone the user's session to allow the vic-machine-server govmomi client to make vSphere requests on the user's behalf.

The govmomi changes in this PR will be moved to a PR in the govmomi repo once I've verified that this is the proper approach.

Fixes #6033

zjs

https://github.com/vmware/vic/pull/6587/files#diff-82bf77133ff3990110da03c8671f97b0L177

AllowedHeaders: []string{"Authorization", "Content-Type", "User-Agent"},

You'll want to add the new header here so that browsers allow it to be sent as a part of cross-origin requests.

zjs

Nice work!

zjs · 2017-10-18T22:16:03Z

lib/apiservers/service/restapi/handlers/common.go

+		b.user = c.user
+		b.password = c.pass
+	} else {
+		b.ticket = p.(Session).ticket


Can we be defensive here and make this } else if s, ok := p.(Session); ok {?

That way we can have an else block so we don't panic if p isn't either.

I think that's a case where you want to panic, as such a case would be a programmer error rather than error due to user input.

Also, we shouldn't really get this far without one or the other as swagger will throw an auth error (though we probably shouldn't rely on that behavior).

zjs · 2017-10-18T22:17:26Z

lib/apiservers/service/restapi/handlers/vch_cert_get.go

+	op := trace.NewOperation(params.HTTPRequest.Context(), "VCHCertGet: %s", params.VchID)
+
+	b := BuildDataParams{
+		url:        url.URL{Host: params.Target},


Since you're adjusting this pattern anyway, maybe BuildDataParams should take a raw target so we can have the url.URL{Host: params.Target} happen in a single place (i.e., buildData).

Good idea, fixed!

zjs · 2017-10-18T22:18:16Z

lib/apiservers/service/restapi/handlers/common.go

@@ -33,26 +33,37 @@ import (
 	"github.com/vmware/vic/pkg/vsphere/vm"
 )

-func buildData(ctx context.Context, url url.URL, user string, pass string, thumbprint *string, datacenter *string, computeResource *string) (*data.Data, error) {
+type BuildDataParams struct {


I'd only expect this to be used within handlers. Does it need to be exported?

Good catch, fixed

zjs · 2017-10-18T22:19:54Z

lib/apiservers/service/restapi/handlers/vch_cert_get.go

-		params.Thumbprint,
-		nil,
-		nil)
+	op := trace.NewOperation(params.HTTPRequest.Context(), "VCHCertGet: %s", params.VchID)


Can you ensure that anywhere that you're introducing an operation, it replaces all occurrences of params.HTTPRequest.Context() (not just the first)?

Fixed, thanks

zjs · 2017-10-18T22:20:55Z

lib/apiservers/service/swagger.json

@@ -830,6 +830,12 @@
    }
  },
  "parameters": {
+    "session-ticket": {


This doesn't seem to be used.

Hmm yeah, it looks like the ticket need only be used in the session security definition. I'll remove and re-test.

dougm

cool, glad CloneSession worked out. quick pass lgtm

andrewtchin · 2017-10-23T16:46:06Z

lib/apiservers/service/restapi/handlers/common.go

+		d.Target.CloneTicket = principal.(Session).ticket
+	}
+
+	if params.thumbprint != nil {


what do we do if there is no thumbprint?

Being able to call the API without passing a thumbprint just requires that the vSphere target has a well-signed, or otherwise trusted, certificate (like calling the CLI without a thumbprint).

This change allows the vic-machine-server to accept a vSphere session clone ticket in the X-VMWARE-TICKET header of incoming requests. This ticket is used by govmomi to clone the user's session to allow the vic-machine-server govmomi client to make vSphere requests on the user's behalf.

jzt requested review from dougm and zjs October 18, 2017 21:48

vmwclabot added the cla-not-required label Oct 18, 2017

jzt force-pushed the vic-machine/6033 branch 2 times, most recently from 8c2529d to 6b09f07 Compare October 18, 2017 21:53

jzt changed the title ~~WIP - Accept session clone ticket via request header~~ [specific ci=Group23-VIC-Machine-Service] Accept session clone ticket via request header Oct 18, 2017

jzt requested a review from lcastellano October 18, 2017 22:00

jzt mentioned this pull request Oct 18, 2017

Update core vic-machine code to allow authentication using a session key, session cookie, or SAML token #6033

Closed

zjs reviewed Oct 18, 2017

View reviewed changes

dougm reviewed Oct 18, 2017

View reviewed changes

jzt force-pushed the vic-machine/6033 branch 4 times, most recently from 57e2ffb to abd7458 Compare October 19, 2017 18:36

zjs approved these changes Oct 20, 2017

View reviewed changes

jzt force-pushed the vic-machine/6033 branch from abd7458 to 6b71cfb Compare October 20, 2017 17:58

jzt removed the request for review from lcastellano October 20, 2017 17:59

andrewtchin reviewed Oct 23, 2017

View reviewed changes

andrewtchin approved these changes Oct 23, 2017

View reviewed changes

zjs force-pushed the feature/vic-machine-service branch from f7667f3 to 4cc1a61 Compare October 25, 2017 00:50

jzt force-pushed the vic-machine/6033 branch 5 times, most recently from 70c771a to a40b159 Compare October 25, 2017 21:03

jzt force-pushed the vic-machine/6033 branch from a40b159 to 618a05e Compare October 25, 2017 21:13

jzt merged commit 91904e9 into vmware:feature/vic-machine-service Oct 26, 2017

zjs mentioned this pull request Oct 31, 2017

Update API to allow authentication using a session key, session cookie, or SAML token #6034

Closed

jzt deleted the vic-machine/6033 branch April 26, 2018 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[specific ci=Group23-VIC-Machine-Service] Accept session clone ticket via request header #6587

[specific ci=Group23-VIC-Machine-Service] Accept session clone ticket via request header #6587

jzt commented Oct 18, 2017

zjs left a comment

zjs left a comment

zjs Oct 18, 2017

dougm Oct 18, 2017

jzt Oct 18, 2017

zjs Oct 18, 2017

jzt Oct 18, 2017

zjs Oct 18, 2017

jzt Oct 18, 2017

zjs Oct 18, 2017

jzt Oct 19, 2017

zjs Oct 18, 2017

jzt Oct 18, 2017

dougm left a comment

andrewtchin Oct 23, 2017

zjs Oct 23, 2017

[specific ci=Group23-VIC-Machine-Service] Accept session clone ticket via request header #6587

[specific ci=Group23-VIC-Machine-Service] Accept session clone ticket via request header #6587

Conversation

jzt commented Oct 18, 2017

zjs left a comment

Choose a reason for hiding this comment

zjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment