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

FDSNWS: implement EIDA token authentication and add authentication to routed Clients #1928

Merged
merged 29 commits into from Oct 16, 2017

Conversation

Projects
None yet
3 participants
@megies
Member

megies commented Oct 13, 2017

This is a simplistic implementation of the EIDA token authentication mechanism, see #1397 and http://geofon.gfz-potsdam.de/waveform/archive/auth/auth-example.php.

Didn't add the tests yet, because I don't get the expected data back, see #1397 (comment).

I think we should include it in 1.1.0, if bugs turn up in the long run we can still fix them in point releases. While this doesn't add a mechanism for token auth in the routed client flavors, at least users can use their tokens when directly talking to the respective data center then.

+TESTS:clients.fdsn
+DOCS

CC @krischer @andres-h

@megies megies added this to the 1.1.0 milestone Oct 13, 2017

@megies megies requested a review from krischer Oct 13, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 13, 2017

Member

If eida_token is given it should check the WADL file if the /auth route is available - and only then use it and otherwise raise an error.

Member

krischer commented Oct 13, 2017

If eida_token is given it should check the WADL file if the /auth route is available - and only then use it and otherwise raise an error.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

At this point the services are not yet discovered.. I would have to drop in a temporary dummy opener and headers to self, then discover services and then replace the dummy opener with the real opener at the end of __init__. I can do that if you want..

Member

megies commented Oct 13, 2017

At this point the services are not yet discovered.. I would have to drop in a temporary dummy opener and headers to self, then discover services and then replace the dummy opener with the real opener at the end of __init__. I can do that if you want..

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 13, 2017

Member

At this point the services are not yet discovered.. I would have to drop in a temporary dummy opener and headers to self, then discover services and then replace the dummy opener with the real opener at the end of init. I can do that if you want..

Any reason this can not just be moved to the end of the init method?

What would also be nice for later usage in the routing services: Client.eida_token as a property that automatically downloads the credentials - then this could be added after the clients have been intialized.

Member

krischer commented Oct 13, 2017

At this point the services are not yet discovered.. I would have to drop in a temporary dummy opener and headers to self, then discover services and then replace the dummy opener with the real opener at the end of init. I can do that if you want..

Any reason this can not just be moved to the end of the init method?

What would also be nice for later usage in the routing services: Client.eida_token as a property that automatically downloads the credentials - then this could be added after the clients have been intialized.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

Right now it's just before the openers are set up using user/password auth mechanism. If I'd want to move it to the bottom I'd have to refactor the normal auth mechanism.. also doable, if you want

Member

megies commented Oct 13, 2017

Right now it's just before the openers are set up using user/password auth mechanism. If I'd want to move it to the bottom I'd have to refactor the normal auth mechanism.. also doable, if you want

@andres-h

This comment has been minimized.

Show comment
Hide comment
@andres-h

andres-h Oct 13, 2017

andres-h commented Oct 13, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 13, 2017

Member

I'd suggest to fall back to no auth instead of raising an error.

When using the routing clients: yes. Otherwise we should definitely raise an error so users are aware something is wrong.

Member

krischer commented Oct 13, 2017

I'd suggest to fall back to no auth instead of raising an error.

When using the routing clients: yes. Otherwise we should definitely raise an error so users are aware something is wrong.

@megies megies changed the title from simple implementation of EIDA token authentication in FDSNWS to FDSNWS: implement EIDA token authentication and add authentication to routed Clients Oct 13, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

Only thing missing is tests for providing credentials to RoutingClient..

.. ah, and I don't check the wadl for auth endpoint yet..

Member

megies commented Oct 13, 2017

Only thing missing is tests for providing credentials to RoutingClient..

.. ah, and I don't check the wadl for auth endpoint yet..

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

@krischer, actually the WADL parser currently mostly looks up methods in the wadl and to determine if auth is supported it needs to check whether it's in resources..

i guess we need a tiny method has_eida_auth() for that on the wadl parser..

screenshot from 2017-10-13 19-38-19

There really should be an official discussion whether this whole mechanism can be added to official FDSN WS specs, this is really quite an unofficial hijack add-on to FDSNWS.

Member

megies commented Oct 13, 2017

@krischer, actually the WADL parser currently mostly looks up methods in the wadl and to determine if auth is supported it needs to check whether it's in resources..

i guess we need a tiny method has_eida_auth() for that on the wadl parser..

screenshot from 2017-10-13 19-38-19

There really should be an official discussion whether this whole mechanism can be added to official FDSN WS specs, this is really quite an unofficial hijack add-on to FDSNWS.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

@andres-h some things i noticed while working on this:

  • the auth endpoint is documented in the dataselect wadl but it's not listed on the dataselect landing page (see https://geofon.gfz-potsdam.de/fdsnws/dataselect/1)
  • there's "localhost" in the wadl, I think we've seen it before (and it should've been fixed by seiscomp?) (assuming there's a seiscomp behind it, can't confirm right now as it's down currently)
  • next: it seems that the GEOFON fdsnws is down quite regularly for a short (but very noticeable) period of time, on the order of minutes (actually it's been down for around an hour right now), see screenshot below. I've encountered this twice now, and I only made some sparse requests for testing throughout the day

One more thing @andres-h, is the auth endpoint exclusive to dataselect?

screenshot from 2017-10-13 19-53-27

Member

megies commented Oct 13, 2017

@andres-h some things i noticed while working on this:

  • the auth endpoint is documented in the dataselect wadl but it's not listed on the dataselect landing page (see https://geofon.gfz-potsdam.de/fdsnws/dataselect/1)
  • there's "localhost" in the wadl, I think we've seen it before (and it should've been fixed by seiscomp?) (assuming there's a seiscomp behind it, can't confirm right now as it's down currently)
  • next: it seems that the GEOFON fdsnws is down quite regularly for a short (but very noticeable) period of time, on the order of minutes (actually it's been down for around an hour right now), see screenshot below. I've encountered this twice now, and I only made some sparse requests for testing throughout the day

One more thing @andres-h, is the auth endpoint exclusive to dataselect?

screenshot from 2017-10-13 19-53-27

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 14, 2017

Member

I've added passing in a per-base-url credentials dictionary to the routed client (also some basic tests for it).

CI is green. Please review @krischer .

Member

megies commented Oct 14, 2017

I've added passing in a per-base-url credentials dictionary to the routed client (also some basic tests for it).

CI is green. Please review @krischer .

@andres-h

This comment has been minimized.

Show comment
Hide comment
@andres-h

andres-h Oct 15, 2017

andres-h commented Oct 15, 2017

@andres-h

This comment has been minimized.

Show comment
Hide comment
@andres-h

andres-h Oct 15, 2017

andres-h commented Oct 15, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 15, 2017

Member

timeouts. Frustratingly, there is an unfound bug that sometimes causes fdsnws to hang during update, so it is restarted by watchdog after 10 minutes.

The 1 hour outage was caused [...]

Alright, that's probably what I was experiencing earlier. Thanks for the information.

Member

megies commented Oct 15, 2017

timeouts. Frustratingly, there is an unfound bug that sometimes causes fdsnws to hang during update, so it is restarted by watchdog after 10 minutes.

The 1 hour outage was caused [...]

Alright, that's probably what I was experiencing earlier. Thanks for the information.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 15, 2017

Member

CI is green. Please review @krischer .

Gonna merge this soon, to finally be able to do the RC.

Member

megies commented Oct 15, 2017

CI is green. Please review @krischer .

Gonna merge this soon, to finally be able to do the RC.

megies added some commits Oct 13, 2017

fdsn: test eida token auth
only tests resolution of user/password from token, does no actual
downloading as we don't have a real test token that grants access to
restricted data. but that's ok since we already have tests for
authenticated queries (i hope ^^) and the auth mechanism of the actual
requests doesn't differ
FDSN EIDA auth: we need to store whether EIDA auth is supported in the
service info dictionary because this is what gets cached and if we don't
store the info there it will be lost and authentication will not be used
in later requests

megies and others added some commits Oct 14, 2017

FDSN routed client: add test case that passing in credentials dictionary
to the routed client works as expected (i.e. credentials are
subsequently used in the actual basic FDSN Client calls that get
executed)
Using built-in downloading method. I know we want to move towards req…
…uests long-term but mixing both in the same client for now seems a bit messy.
Exception instead of warning.
This is probably some user error and so they should be forced to fix it.
@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 15, 2017

Member

Feel free to merge from my point of view if CI passes.

The routers credentials now work like this:

c = RoutingClient(..., credentials={
    # Uses ObsPy's URL mappings + user/pw combination.
    'IRIS': {'user': 'bla', 'password': 'bla2'},
    # per fdsnws url token.
    'geofon.gfz-postdam.de': {'eida_token': '/path/to/token'},
    # Global token - will be tried for every datacenter that has the 
    # /auth route and does not have data center specific settings
    'EIDA_TOKEN': '/path/to/other/token'}

Currently the /auth route times out for every data center except the GFZ one. So this might take a while before it is an actually useful feature.

Member

krischer commented Oct 15, 2017

Feel free to merge from my point of view if CI passes.

The routers credentials now work like this:

c = RoutingClient(..., credentials={
    # Uses ObsPy's URL mappings + user/pw combination.
    'IRIS': {'user': 'bla', 'password': 'bla2'},
    # per fdsnws url token.
    'geofon.gfz-postdam.de': {'eida_token': '/path/to/token'},
    # Global token - will be tried for every datacenter that has the 
    # /auth route and does not have data center specific settings
    'EIDA_TOKEN': '/path/to/other/token'}

Currently the /auth route times out for every data center except the GFZ one. So this might take a while before it is an actually useful feature.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 16, 2017

Member

Currently the /auth route times out for every data center except the GFZ one. So this might take a while before it is an actually useful feature.

@andres-h Is this how the tokens are supposed to be used? And how are the datacenters supposed to behave?

If a global EIDA_TOKEN key is given as part of a routing client's credentials it will be applied to every datacenter that claims to implement the /auth route. But only the GFZ node does not time out.

Member

krischer commented Oct 16, 2017

Currently the /auth route times out for every data center except the GFZ one. So this might take a while before it is an actually useful feature.

@andres-h Is this how the tokens are supposed to be used? And how are the datacenters supposed to behave?

If a global EIDA_TOKEN key is given as part of a routing client's credentials it will be applied to every datacenter that claims to implement the /auth route. But only the GFZ node does not time out.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 16, 2017

Member

I will merge this right now.. looks good in general with me and there's only the question left if we should use the token globally for all EIDA datacenters or not. If we're on the wrong track in that regard, we can still change it on master before the release.

Member

megies commented Oct 16, 2017

I will merge this right now.. looks good in general with me and there's only the question left if we should use the token globally for all EIDA datacenters or not. If we're on the wrong track in that regard, we can still change it on master before the release.

@megies megies merged commit 6d3842f into master Oct 16, 2017

5 of 7 checks passed

docs-buildbot Build succeeded, but there are warnings/errors:
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 95.68% of diff hit (target 90%)
Details
codecov/project 88.54% (+1.56%) compared to 43fbdd5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@megies megies deleted the eida_token_auth branch Oct 16, 2017

@andres-h

This comment has been minimized.

Show comment
Hide comment
@andres-h

andres-h Oct 16, 2017

andres-h commented Oct 16, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 16, 2017

Member

Thanks for the clarification. Then we will leave it as it currently is and change it if required in the future.

Member

krischer commented Oct 16, 2017

Thanks for the clarification. Then we will leave it as it currently is and change it if required in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment