search

Ecwid / consul-api

Java client for Consul HTTP API
Apache License 2.0
416 stars 177 forks source link

Session creation is not possible with acl token #102

Open zeldigas opened 7 years ago

zeldigas commented 7 years ago

It's not possible now to invoke session operations with ACL token, although according to consul docs session operations are also subject to ACL control.

Current problem is that it's not possible to pass token as query parameters, because this class is final, and as non-raw client does not accept URLParameters interface it's not possible to pass custom values.

P.S. Probably it's a good idea to include token to QueryParams object as it's now missing not only for session part of API.

krystianekb commented 7 years ago

Also the agent endpoint and deregistration are now acl-token aware on Consul side but not fully supported by the library. One thing to take into account is that in the recent Consul version (0.8.3) the recommended way for passing the token is via http header and not url query param.

Accroding to consul API: "Here is an example using curl:

$ curl \ --header "X-Consul-Token: abcd1234" \ https://consul.rocks/v1/agent/members Previously this was provided via a ?token= query parameter. This functionality exists on many endpoints for backwards compatibility, but its use is highly discouraged, since it can show up in access logs as part of the URL."

zeldigas commented 7 years ago

@vgv would be nice to hear from you - is there any plans to support that from your side or you are expecting pull request here

vgv commented 7 years ago

@zeldigas Hi, I definitely will add support for tokens in all session related operations, but I do not plan to do it now. How urgent do you need it?

vgv commented 7 years ago

About PR - I always like pull requests :) However, for this feature it is better DO NOT send me PR, because I want to rework a bit code in *.transport package at one.

zeldigas commented 7 years ago

@vgv it's not super urgent, as for my purpose I already did a local hack that allows to add token to queryParameters, but as @krystianekb mentioned it's not the best way of doing it. So once you implement it in well structured way, I'll switch to your approach and remove mine :)

krystianekb commented 7 years ago

Unfortuntely I am also a bit loaded now (hopefully beginning July I will have more time on this), we applied also a workaround in the project but I believe the changes should be done in two stages:

  1. Passing token as query parameters - as existing Consul fully supports it even though discourages from using it
  2. Pass the token as HTTP header value (X-Consul-Token) In that approach we would have it fully functional after phase 1 and then extended to longer term strategy after phase 2
zeldigas commented 6 years ago

hey @vgv any news on that issue?

sweeneyb commented 6 years ago

I'm running up against this, as well. I'm looking to upgrade to consul 1.0.x with ACLs on everything. As it stands now, I think I'm going to have to make my "acl_token" configured special token contain session::write instead of session::read, which isn't ideal.