storwize driver doesn't handle spaces in server names

[Zoltan Arnold Nagy]

If there's a space in the server's name, the storwize driver's SSH command will get caught by the SSH injection check in cinder.

[OpenStack Infra (hudson-openstack)]

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

[John Griffith (john-griffith)]

This is exactly what the injection check is for

[Zoltan Arnold Nagy (zoltan)]

As I noted it in the review, without this change, there are valid storage configurations that cannot be managed with the cinder driver, making it not usable in these environments.

[Jay Bryant (jsbryant)]

Zoltan, I do not believe the hostname you have provided in your example is a legal hostname. From Wikipedia:

The Internet standards (Request for Comments) for protocols mandate that component hostname labels may contain only the ASCII letters 'a' through 'z' (in a case-insensitive manner), the digits '0' through '9', and the hyphen ('-'). The original specification of hostnames in RFC 952, mandated that labels could not start with a digit or with a hyphen, and must not end with a hyphen. However, a subsequent specification (RFC 1123) permitted hostname labels to start with digits. No other symbols, punctuation characters, or white space are permitted.

Allowing spaces in the ssh commands opens the dangerous security holes.

[Eric Harney (eharney)]

This looks like a reasonable fix to me. Whether hostnames are allowed to have spaces in them is irrelevant for what this patch is actually fixing.

I think the patch just needs a title that isn't talking about SSH injection because it sends the wrong message. The goal isn't to prevent an SSH injection attack.

[John Griffith (john-griffith)]

Eric,
I guess I"m confused because IMO what this patch does is first of all allow the use of an invalid hostname, and second does it in such a way as to bypass our injection check, so it very much IS related to ssh and injection regardless of the title.

So if I set the hostname to: "some-host; rm -rf /" for example... that seems like a problem to me and I believe that's the sort of thing the injection check is actually there for. Bypassing the injection check to allow spaces in on commands seems like a very bad idea, inparticular for a case that isn't even valid.

Maybe you can explain in detail why you think this is a reasonable fix and more importantly why it's needed? I seem to be missing some detail here that maybe you're aware of or have noticed.

[Zoltan Arnold Nagy (zoltan)]

John,

Your confusion comes from the misleading variable name I think in the driver. It's not a valid, RFC defined hostname. It's a server name that references a fiber channel ID in the driver (so you can map volumes to hosts). Now, this server name can be arbitrarily defined by the user.

If you were to set the hostname to "some host; rm -rf /", that would be indeed caught even after the patches by the SSH injection check (and I'm sure it the storwize system wouldn't allow that in the first place, so we wouldn't even get this far). The only character that the injection check allows is a space, and it only allows that if it's within quotes.

So even if you somehow managed to set that hostname through the storwize CLI/GUI (or FSM CLI/GUI), which is doubtful, it won't be sent to the storage.

(And even if it were sent to the storage, the other size of it's SSH connection is an rbash, where you cannot misbehave or do anything distruptive at all.)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/67507
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=e62b70562dd8d2d94b526f3f7a52ef4aff9a3f40
Submitter: Jenkins
Branch: master

commit e62b70562dd8d2d94b526f3f7a52ef4aff9a3f40
Author: Avishay Traeger <email address hidden>
Date: Sun Jan 26 12:39:27 2014 +0200

    Allow spaces in host names in the storwize driver

    Storwize/SVC naming rules allow host objects to have spaces in their
    names. The SSH injection filter will currently reject such names, and so
    we surround the names with quotes. Please note that this doesn't allow
    to have an arbitrary character in there except a space (anything else
    will be cleaned up by the driver or rejected by the SSH injection filter
    up the call chain).

    Change-Id: If9aec40fe34293031f08d759dd930d73ead456f5
    Closes-Bug: #1270204