Multiple domains support

[BlockListed]

Fixes #2690

Very WIP PR, I just want some feedback about my approach for now.

I am planning to go with the allowed domains approach.

Overview:

• ~~We create 2 Hashmaps, which map the Host header to either Domain or Origin.~~
• We create a hashmap, which maps the Host header to a combination of Domain and Origin.

Limitations:

• All domains have to have the same path, because otherwise we would need one web server instance for each different path.

Future: ~~- Change JWT system to create tokens, which work for a single domain.~~

[BlackDex]

My first question would be, why change Host to Domain? That is not how the header is called.

And a dev tip, install and activate pre-commit, that will help fixing issues before committing 😄

[BlockListed]

first question would be, why change Host to Domain? That is not how the header is called.

And a dev tip, install and activate pre-commit, that will help fixing issues before committing 😄

Host

Because it definitely isn't a host.

format!("{protocol}://{host}")

This is a base URL, but you could say, that domain might also not be the correct term. My reasoning was, that domain is the term used for the config. It is used when creating URLs and it was very confusing. (for me at least) I'd have no problem changing it back tho.

Pre-Commit

I ran cargo clippy, I know it's definitely not finished yet, but I'm not gonna put in all that work and find out at the end, that my approach is fully incompatible with the vision/goals of the project maintainers.

[stefan0xC]

Because it definitely isn't a host.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host

[BlockListed]

Because it definitely isn't a host.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host

It does not store a host. It stores a base url.

If it stored a host the extractor would be:

let headers = ...;

if let Some(host) = headers.get_one("X-Forwarded-Host") {
    host
} else if let Some(host) headers.get_one("Host") {
    host
} else {
    panic!("invalid http request");
}

But it's not, the extractor gets a URL from a combination of protocol, host and path (from config.domain()).

It doesn't store developer.mozilla.org (from Host: developer.mozilla.org) it would store https://developer.mozilla.org.

[BlackDex]

It definitely isn't a domain, a domain is the last 2 (or 3) parts. So, a domain is vaultwarden.org, a host is vault.vaultwarden.org, so what we are receiving is actually a host.

Edit: so actually, our config is wrong too 😅

[BlockListed]

Ot definitely isn't a domain, a domain is the last 2 (or 3) parts. So, a domain is vaultwarden.org, a host is vault.vaultwarden.org, so what we are receiving is actually a host.

I know, but the precedent is set in config.rs

/// Domain URL |> This needs to be set to the URL used to access the server, including 'http[s]://'
/// and port, if it's different than the default. Some server functions don't work correctly without this value
domain:                 String, true,   def,    "http://localhost".to_string();

This definitely isn't a domain, but that's what the precedent says.

We combine a host with a protocol and possibly a path into a URL.

[BlackDex]

Ot definitely isn't a domain, a domain is the last 2 (or 3) parts. So, a domain is vaultwarden.org, a host is vault.vaultwarden.org, so what we are receiving is actually a host.

I know, but the precedent is set in config.rs

/// Domain URL |> This needs to be set to the URL used to access the server, including 'http[s]://'
/// and port, if it's different than the default. Some server functions don't work correctly without this value
domain:                 String, true,   def,    "http://localhost".to_string();

This definitely isn't a domain, but that's what the precedent says.

I understand, but the location where you changed it has nothing to do with config, but with the headers.

[BlockListed]

Ot definitely isn't a domain, a domain is the last 2 (or 3) parts. So, a domain is vaultwarden.org, a host is vault.vaultwarden.org, so what we are receiving is actually a host.

I know, but the precedent is set in config.rs

/// Domain URL |> This needs to be set to the URL used to access the server, including 'http[s]://'
/// and port, if it's different than the default. Some server functions don't work correctly without this value
domain:                 String, true,   def,    "http://localhost".to_string();

This definitely isn't a domain, but that's what the precedent says.

I understand, but the location where you changed it has nothing to do with config, but with the headers.

So then the correct term would be BaseURL right?? Because what the Host / Domain struct stores is definitely not a host.

I renamed it, because in the optimal situation (the domain is configured) the struct Stores the value from the domain configuration variable.

But all I really want to know is if this approach (looking up the domain by host), not implementation, vibes with the project vision.

[BlackDex]

But, maybe we need to wait for the whole picture. But because we didn't do it correctly in one place, doesn't mean we shouldn't at others or improve. Else we should just make it more clear using comments for other developers maybe. But i would prefer to use the right naming where possible when we adjust the code.

[BlackDex]

There are some issues. Mails can be sent without any HTTP connection, so there must be one main host/domain in the config which will be used in those cases. Using a different host/domain for mails when there is a HTTP connection will become confusing.

So, the only location where i think it will be useful for are MFA and attachments/sends.

[BlockListed]

There are some issues. Mails can be sent without any HTTP connection, so there must be one main host/domain in the config which will be used in those cases. Using a different host/domain for mails when there is a HTTP connection will become confusing.

So, the only location where i think it will be useful for are MFA and attachments/sends.

That's why I added pub fn main_domain(&self) -> String to Config, it should probably be more clear in the configuration though which domain is the main one. (Currently the first one)

[BlockListed]

Checks are passing now, but I still have some cleanup to be done. Probably will do it tomorrow, after that it should be ready for review.

To do (at least):

• [x] Remove purposeful breaking changes
• [x] Re-add domain_origin / domain_path options
• [x] Do proper error handling for HostInfo extractor

Behaviours which may need to change (I think they're fine)

• Emails always use the first domain
• JWT issuer is always the first domain's origin

[BlockListed]

From my perspective I would say this is ready to review.

[BlockListed]

@BlackDex Sorry for the ping, but is anything gonna happen with this PR? Because if not, it should probably just be closed.

[BlackDex]

It has to wait for a check. As i mentioned in a few other pr's I'm busy with other stuff (for this project). But i just don't have much time currently.

Just be patient and my comments (or other people's comments) will follow eventually.