create endpoint requests with invalid URLs are not rejected

Bug #1471034 reported by Theodore Ilie
26
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Fix Released
Low
jiaxi

Bug Description

When creating an endpoint, it is possible to have an invalid URL by adding a space in "tenant_id."

keystone endpoint-create --service_id 410de5fed7544c90a36ebdb8b38f0cea --publicurl "http://127.0.0.1:8774 /v1.1/\$(tenant_i d)s" --adminurl " http://127.0.0.1:8774 /v1.1/\$(tenant_i d)s" --internalurl "http://127.0.0.1:8774 /v1.1/\$(tenant_i d)s"

It's your own fault if you make an endpoint with a space there, but it would still be nice for Keystone to stop you and give an error message such as, "Cannot create an endpoint with an invalid URL."

Note: This is NOT a duplicate of bug #1098564. This bug is fairly similar to https://bugs.launchpad.net/keystone/+bug/1098564, but that bug report is about not being able to delete the endpoint. I have a patch in progress that adds a test case for deleting the endpoint. My report is about suppressing invalid URLs; the other one is about deleting an endpoint with an invalid URL. These should have separate patches.

description: updated
Changed in keystone:
importance: Undecided → Low
Nathan Jewell (ncjewell)
Changed in keystone:
assignee: nobody → Nathan Jewell (ncjewell)
Nathan Jewell (ncjewell)
Changed in keystone:
assignee: Nathan Jewell (ncjewell) → nobody
Revision history for this message
Malini Bhandaru (malini-k-bhandaru) wrote :

Are tenant_ids with a space valid? For that matter, are other characters that can break a url valid?
I think the fix lies at the point where tenant_ids may be created. This invalid url is just a side effect.

jiaxi (tjxiter)
Changed in keystone:
assignee: nobody → jiaxi (tjxiter)
Revision history for this message
Theodore Ilie (theoilie-ti) wrote :

I think the problem is that tenant_id is a variable/placeholder that is supposed to be replaced with the actual id of a tenant. So when the placeholder is tenant_i d, it cannot be replaced because the program is searching for the exact string "tenant_id" with no space. The same goes for endpoint_id, but I'm not sure exactly what other characters might break a URL.

Revision history for this message
Dolph Mathews (dolph) wrote :

I'm not sure how to improve the user feedback here as we're already providing error messages at the time we try to parse URLs... which is the only time we have the data available to do string formatting with them. Is there's a standard way to validate a string is ready to be formatted without actually doing so?

tags: added: user-experience
jiaxi (tjxiter)
Changed in keystone:
status: New → In Progress
Revision history for this message
jiaxi (tjxiter) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/#/c/200512/

Revision history for this message
Steve Martinelli (stevemar) wrote :

wouldn't this be *much* easier to solve by making the clients smarter?

Revision history for this message
jiaxi (tjxiter) wrote :

I can solve the bug in the server side throughly without changing the client side.
If I solve the it in the client side.Some code of the server side must be modified.
Because some testcase user uuid string as url in the server client.

And not matter I solve it in the client or server side. The logical is the same.
The code is almost the same.

So I think it's more reasonable and properer to solve it in the server side.

Revision history for this message
jiaxi (tjxiter) wrote :

Hello,everyone:

I commited this patch set https://review.openstack.org/#/c/200512/ mainly for suggestion.

keystone.common.validation.parameter_types.py #57 is terribly wrong.
It can't recognize many invalid urls.
Why should we use this regex?

Revision history for this message
jiaxi (tjxiter) wrote :

As identity v3 has the same problem. Dolph Mathews strongly suggest me to use this logic to valid v3 endpoint url within this patch set.
I need opinions from more cores and more reviewers about whether should I do it in v3 within this patch set.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (master)

Fix proposed to branch: master
Review: https://review.openstack.org/204952

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on keystone (master)

Change abandoned by jiaxi (<email address hidden>) on branch: master
Review: https://review.openstack.org/200512
Reason: Submit a new one.This one run into troble.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by jiaxi (<email address hidden>) on branch: master
Review: https://review.openstack.org/204952

Changed in keystone:
assignee: jiaxi (tjxiter) → Adam Young (ayoung)
Adam Young (ayoung)
Changed in keystone:
assignee: Adam Young (ayoung) → jiaxi (tjxiter)
summary: - invalid URLs are not suppressed
+ create endpoint requests with invalid URLs are not rejected
Changed in keystone:
assignee: jiaxi (tjxiter) → David Stanek (dstanek)
jiaxi (tjxiter)
Changed in keystone:
assignee: David Stanek (dstanek) → jiaxi (tjxiter)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (master)

Reviewed: https://review.openstack.org/200512
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=312d6c9fd0ab3901cafece0fcb4181490910ea65
Submitter: Jenkins
Branch: master

commit 312d6c9fd0ab3901cafece0fcb4181490910ea65
Author: jiaxi <email address hidden>
Date: Thu Jul 23 04:33:13 2015 -0400

    Reject create endpoint with invalid urls

    The patch is not for general URL validation. It just implements
    a check to make sure any substitutions in the URL will be valid
    when building the catalog.

    The bug also happen in v3. So we have to fix this bug in both
    v2 and v3.

    Closes-Bug: #1471034
    Change-Id: I6728edaae26b140fc19afec6331674c57856985c

Changed in keystone:
status: In Progress → Fix Committed
Changed in keystone:
milestone: none → liberty-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in keystone:
milestone: liberty-3 → 8.0.0
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers