Skip to content

Validate instance hostnames in the Nexus API - #4938

Merged
bnaecker merged 9 commits into
mainfrom
validate-instance-hostnames
Feb 1, 2024
Merged

bnaecker merged 9 commits into
mainfrom
validate-instance-hostnames

Conversation

@bnaecker

Copy link
Copy Markdown
Collaborator
  • Add the Hostname type to the external API, which checks that a string conforms to RFC 1035 (and others) describing valid Internet hostnames. Add some tests for that.
  • Use the new type everywhere in Nexus and its integration tests, though we still serialize to a String in the database and when passing the instance information to the sled-agent. The sled-agent does its own validation internally through OPTE.
  • Add a few regression tests for creating instances with known-bad hostnames.

@bnaecker
bnaecker requested review from smklein and taspelund January 30, 2024 23:57
@bnaecker

Copy link
Copy Markdown
Collaborator Author

I'm beefing up the tests here, I think these are not actually testing what I want.

- Add the `Hostname` type to the external API, which checks that a
  string conforms to RFC 1035 (and others) describing valid Internet
  hostnames. Add some tests for that.
- Use the new type everywhere in Nexus and its integration tests, though
  we still serialize to a String in the database and when passing the
  instance information to the sled-agent. The sled-agent does its own
  validation internally through OPTE.
- Add a few regression tests for creating instances with known-bad
  hostnames.
@bnaecker
bnaecker force-pushed the validate-instance-hostnames branch from 4388023 to 8e4e4ad Compare January 31, 2024 18:39
@bnaecker

Copy link
Copy Markdown
Collaborator Author

Ok folks, this is ready for a review.

Comment thread common/src/api/external/mod.rs Outdated
Comment thread nexus/db-queries/src/db/datastore/instance.rs

@smklein smklein left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current build failures are real;

end-to-end-tests/src/instance_launch.rs:67

looks like it's using a hostname. Might not be built by default within the workspace?

@bnaecker

Copy link
Copy Markdown
Collaborator Author

Thanks Sean, nice catch. Fixed.

Comment thread openapi/nexus.json Outdated
@karencfv

Copy link
Copy Markdown
Contributor

Heya @bnaecker, I think you forgot to generate the openapi/nexus.json file in this last commit 0b2a761. This would make the tests fail I think

Comment thread nexus/src/app/instance.rs Outdated
- Deserialize hostname from an owned string
- Fix hostname in end-to-end-tests
@bnaecker

bnaecker commented Feb 1, 2024

Copy link
Copy Markdown
Collaborator Author

Ok, I've run a quick test locally to confirm that this does what I expect. It's a bit nasty, hold your hat.

I launched the control plane as usual on my dev machine, using omicron-package {package,install}. Once things were up, I created all the necessary preconditions: quotas, IP pool linked to the silo, project, Alpine-based image and disk. I then made an instance, which with the new API validation requires a valid hostname:

bnaecker@shale : ~/omicron $ oxide instance list --project $PROJECT
Instance {
    description: "my inst",
    hostname: "test",
    id: 0d027324-bce4-4498-b00d-eea57ac0fa64,
    memory: ByteCount(
        1073741824,
    ),
    name: Name(
        "test",
    ),
    ncpus: InstanceCpuCount(
        2,
    ),
    project_id: 23afd905-24e3-4222-9e37-47836a18697d,
    run_state: Stopped,
    time_created: 2024-02-01T17:49:09.254629Z,
    time_modified: 2024-02-01T17:49:09.254629Z,
    time_run_state_updated: 2024-02-01T17:49:09.254629Z,
}

Note that I created the instance but did not start it. Now, I went into the DB and manually dorked with the hostname, to make it invalid. This should simulate exactly the case of a user having previously created an instance with an invalid hostname, and the applying the offline update past this Omicron commit.

bnaecker@shale : ~/omicron $ ./tools/cockroachdb/bin/cockroach sql --url postgresql://[fd00:1122:3344:101::6]:32221/omicron?sslmode=disable --insecure
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
# Client version: CockroachDB OSS v21.1.10 (x86_64-pc-solaris2.11, built 2021/10/12 22:52:33, go1.16.5)
# Server version: CockroachDB OSS v22.1.9-dirty (x86_64-pc-solaris2.11, built 2022/10/26 21:17:46, go1.17.13)
# Cluster ID: cf550c23-a61a-46f6-88de-c63703c144af
#
# Enter \? for a brief introduction.
#
root@[fd00:1122:3344:101::6]:32221/omicron> select hostname from instance where id = '0d027324-bce4-4498-b00d-eea57ac0fa64';
  hostname
------------
  test
(1 row)

Time: 2ms total (execution 1ms / network 0ms)
root@[fd00:1122:3344:101::6]:32221/omicron> UPDATE instance SET hostname = 'well_shit' WHERE id = '0d027324-bce4-4498-b00d-eea57ac0fa64';
UPDATE 1

Time: 8ms total (execution 8ms / network 0ms)

root@[fd00:1122:3344:101::6]:32221/omicron> select hostname from instance where id = '0d027324-bce4-4498-b00d-eea57ac0fa64';
  hostname
-------------
  well_shit
(1 row)

Time: 1ms total (execution 1ms / network 0ms)

root@[fd00:1122:3344:101::6]:32221/omicron> ^D
bnaecker@shale : ~/omicron $ oxide instance list --project $PROJECT
Instance {
    description: "my inst",
    hostname: "well_shit",
    id: 0d027324-bce4-4498-b00d-eea57ac0fa64,
    memory: ByteCount(
        1073741824,
    ),
    name: Name(
        "test",
    ),
    ncpus: InstanceCpuCount(
        2,
    ),
    project_id: 23afd905-24e3-4222-9e37-47836a18697d,
    run_state: Stopped,
    time_created: 2024-02-01T17:49:09.254629Z,
    time_modified: 2024-02-01T17:49:09.254629Z,
    time_run_state_updated: 2024-02-01T17:49:09.254629Z,
}

Now let's start the instance:

bnaecker@shale : ~/omicron $ oxide instance start --instance 0d027324-bce4-4498-b00d-eea57ac0fa64
error
Error Response: status: 400 Bad Request; headers: {"content-type": "application/json", "x-request-id": "2fa558fa-e79f-4557-9972-f5168bd69730", "content-length": "322", "date": "Thu, 01 Feb 2024 17:52:36 GMT"}; value: Error { error_code: Some("InvalidRequest"), message: "The instance hostname 'well_shit' is no longer valid. To access the data on its disks, this instance must be deleted, and a new one created with the relevant disks. The new hostname will be validated at that time.", request_id: "2fa558fa-e79f-4557-9972-f5168bd69730" }

So, we're at least preventing users from torpedoing an entire sled, by triggering #4931. We're also providing a reasonably clear set of steps to recover the instance's data, if needed.

@bnaecker
bnaecker requested a review from jmpesp February 1, 2024 18:09

@smklein smklein left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New approach LGTM too!

@bnaecker
bnaecker enabled auto-merge (squash) February 1, 2024 18:36
@bnaecker

bnaecker commented Feb 1, 2024

Copy link
Copy Markdown
Collaborator Author

Failed the OpenAPI check yet again, since the new commits return a String as the hostname when fetching an instance.

@jmpesp jmpesp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nexus should validate hostnames in the API

4 participants