Deuxfleurs/tricot
17
3
Fork 3
Code Issues 4 Pull requests Projects Releases Wiki Activity

Add support for domain checks. #11

Merged
quentin merged 3 commits from domain-check into main 2023-11-30 17:07:58 +00:00
Conversation 1 Commits 3 Files changed 2 +158 -39
quentin commented 2023-11-30 14:36:03 +00:00
Owner
Copy link

Fixes #8

  • Feature spec
  • Feature code
  • Test it
  • Deploy it
  • Add directive to the garage service

Testing feature

Test setup

cd /tmp/a
python3 -m http.server --bind 0.0.0.0 8000

cd /tmp/b
touch check
python3 -m http.server --bind 0.0.0.0 3903
 
consul services register \
  -name test \
  -address=localhost \
  -port 8000 \
  -tag="tricot *.localhost" \
  -tag="tricot-on-demand-tls-ask http://localhost:3903/check"

Happy path:

2023-11-30T16:37:36.153666Z  INFO tricot::cert_store: (a.localhost) On-demand TLS check
2023-11-30T16:37:36.160850Z TRACE tricot::cert_store: domain a.localhost validated on glob pattern *.localhost and on check url http://localhost:3903/check
2023-11-30T16:37:36.160920Z DEBUG tricot::cert_store: Checking cert for domain: a.localhost
2023-11-30T16:37:36.161730Z  INFO tricot::cert_store: (a.localhost) Renewing certificate

The service does not answer:

2023-11-30T16:38:13.936107Z  INFO tricot::cert_store: (b.localhost) On-demand TLS check
2023-11-30T16:38:13.937581Z  WARN tricot::cert_store: domain b.localhost validation refused  on glob pattern *.localhost and on check url http://localhost:3903/check with error: error sending request for url (http://localhost:3903/check?domain=b.localhost): error trying to connect: tcp connect error: Connection refused (os error 111)

When the server does not return a 200 code (404 here):

2023-11-30T16:38:49.581892Z  INFO tricot::cert_store: (c.localhost) On-demand TLS check
2023-11-30T16:38:49.588851Z  WARN tricot::cert_store: domain c.localhost validation refused  on glob pattern *.localhost and on check url http://localhost:3903/check with error: c.localhost is not authorized for on-demand TLS

Non regresion

Test setup

python3 -m http.server
 
consul services register \
  -name test \
  -address=localhost \
  -port 8000 \
  -tag="tricot *.localhost"

Happy path:

2023-11-30T16:40:25.426590Z TRACE tricot::cert_store: domain a.localhost validated on glob pattern *.localhost only
2023-11-30T16:40:25.426617Z DEBUG tricot::cert_store: Checking cert for domain: a.localhost

Test setup 2 :

consul services register \
  -name test \
  -address=localhost \
  -port 8000 \
  -tag="tricot *.b.localhost"

Only x.b.localhost is accepted

2023-11-30T16:42:18.275459Z DEBUG tricot::https: TLS handshake was successfull
2023-11-30T16:42:18.276063Z DEBUG tricot::https: a.localhost/ -> NOT FOUND
...
2023-11-30T16:42:21.076918Z DEBUG tricot::https: TLS handshake was successfull
2023-11-30T16:42:21.077517Z DEBUG tricot::https: b.localhost/ -> NOT FOUND
...
2023-11-30T16:42:23.899172Z  INFO tricot::cert_store: Added self-signed certificate for x.b.localhost
2023-11-30T16:42:23.900251Z TRACE tricot::cert_store: domain x.b.localhost validated on glob pattern *.b.localhost only
2023-11-30T16:42:23.900278Z DEBUG tricot::cert_store: Checking cert for domain: x.b.localhost
Fixes #8 - [X] Feature spec - [X] Feature code - [x] Test it - [x] Deploy it - [X] Add directive to the garage service ## Testing feature Test setup ```bash cd /tmp/a python3 -m http.server --bind 0.0.0.0 8000 cd /tmp/b touch check python3 -m http.server --bind 0.0.0.0 3903 consul services register \ -name test \ -address=localhost \ -port 8000 \ -tag="tricot *.localhost" \ -tag="tricot-on-demand-tls-ask http://localhost:3903/check" ``` Happy path: ``` 2023-11-30T16:37:36.153666Z INFO tricot::cert_store: (a.localhost) On-demand TLS check 2023-11-30T16:37:36.160850Z TRACE tricot::cert_store: domain a.localhost validated on glob pattern *.localhost and on check url http://localhost:3903/check 2023-11-30T16:37:36.160920Z DEBUG tricot::cert_store: Checking cert for domain: a.localhost 2023-11-30T16:37:36.161730Z INFO tricot::cert_store: (a.localhost) Renewing certificate ``` The service does not answer: ``` 2023-11-30T16:38:13.936107Z INFO tricot::cert_store: (b.localhost) On-demand TLS check 2023-11-30T16:38:13.937581Z WARN tricot::cert_store: domain b.localhost validation refused on glob pattern *.localhost and on check url http://localhost:3903/check with error: error sending request for url (http://localhost:3903/check?domain=b.localhost): error trying to connect: tcp connect error: Connection refused (os error 111) ``` When the server does not return a 200 code (404 here): ``` 2023-11-30T16:38:49.581892Z INFO tricot::cert_store: (c.localhost) On-demand TLS check 2023-11-30T16:38:49.588851Z WARN tricot::cert_store: domain c.localhost validation refused on glob pattern *.localhost and on check url http://localhost:3903/check with error: c.localhost is not authorized for on-demand TLS ``` ## Non regresion Test setup ```bash python3 -m http.server consul services register \ -name test \ -address=localhost \ -port 8000 \ -tag="tricot *.localhost" ``` Happy path: ``` 2023-11-30T16:40:25.426590Z TRACE tricot::cert_store: domain a.localhost validated on glob pattern *.localhost only 2023-11-30T16:40:25.426617Z DEBUG tricot::cert_store: Checking cert for domain: a.localhost ``` Test setup 2 : ``` consul services register \ -name test \ -address=localhost \ -port 8000 \ -tag="tricot *.b.localhost" ``` Only x.b.localhost is accepted ``` 2023-11-30T16:42:18.275459Z DEBUG tricot::https: TLS handshake was successfull 2023-11-30T16:42:18.276063Z DEBUG tricot::https: a.localhost/ -> NOT FOUND ... 2023-11-30T16:42:21.076918Z DEBUG tricot::https: TLS handshake was successfull 2023-11-30T16:42:21.077517Z DEBUG tricot::https: b.localhost/ -> NOT FOUND ... 2023-11-30T16:42:23.899172Z INFO tricot::cert_store: Added self-signed certificate for x.b.localhost 2023-11-30T16:42:23.900251Z TRACE tricot::cert_store: domain x.b.localhost validated on glob pattern *.b.localhost only 2023-11-30T16:42:23.900278Z DEBUG tricot::cert_store: Checking cert for domain: x.b.localhost ```
quentin added 1 commit 2023-11-30 14:36:03 +00:00
some comments
Some checks failed
continuous-integration/drone/push Build is failing
Details
continuous-integration/drone/pr Build is failing
Details
ca449ebff4
quentin added 1 commit 2023-11-30 15:53:10 +00:00
implement feature
All checks were successful
continuous-integration/drone/pr Build is passing
Details
continuous-integration/drone/push Build is passing
Details
753903ee02
quentin force-pushed domain-check from 346d51ed93 to b9b035034f 2023-11-30 16:35:24 +00:00 Compare
quentin changed title from WIP: Add support for domain checks. to Add support for domain checks. 2023-11-30 16:43:42 +00:00
quentin merged commit 388d5b2275 into main 2023-11-30 17:07:58 +00:00
quentin deleted branch domain-check 2023-11-30 17:07:58 +00:00
lx reviewed 2023-11-30 17:21:55 +00:00
src/cert_store.rs
@ -104,4 +136,4 @@
match t_last_check.get(dom) {
Some(t) if Instant::now() - *t < Duration::from_secs(60) => continue,
_ => t_last_check.insert(dom.to_string(), Instant::now()),
};
lx commented 2023-11-30 17:18:10 +00:00
Owner
Copy link

Peut-être que ça serait pertinent de déplacer le call de domain_validation ici (après ces lignes), pour bénéfiicer de la logique qui fait que des domaines sont pas re-checkés en moins de 60 seconds ?

Peut-être que ça serait pertinent de déplacer le call de `domain_validation` ici (après ces lignes), pour bénéfiicer de la logique qui fait que des domaines sont pas re-checkés en moins de 60 seconds ?
quentin commented 2023-11-30 18:16:56 +00:00
Author
Owner
Copy link

C'est vrai, c'est que j'ai refactor tant de fois mon code que ça ne m'a pas sauté aux yeux. Mais carrément !

C'est vrai, c'est que j'ai refactor tant de fois mon code que ça ne m'a pas sauté aux yeux. Mais carrément !
src/cert_store.rs
@ -127,0 +158,4 @@
let proc_domains = match maybe_proc_domains {
None => {
warn!("Proxy config is not yet loaded, refusing all certificate generation");
return domains;
lx commented 2023-11-30 17:16:13 +00:00
Owner
Copy link

Du coup ici ça devrait plutôt être return HashSet::new() non?

Du coup ici ça devrait plutôt être `return HashSet::new()` non?
quentin commented 2023-11-30 18:16:23 +00:00
Author
Owner
Copy link

Il me semble que c'est équivalent non ? domains est intialisé avec HashSet::new() juste au dessus et on a pas eu l'occasion de le modifier.

Il me semble que c'est équivalent non ? domains est intialisé avec HashSet::new() juste au dessus et on a pas eu l'occasion de le modifier.
src/cert_store.rs
@ -127,0 +179,4 @@
}
// It's not a static domain, maybe an on-demand domain?
for (pattern, maybe_check_url) in proc_domains.on_demand_domains.iter() {
lx commented 2023-11-30 17:20:58 +00:00
Owner
Copy link

Pour éviter la double boucle et de devoir faire des break 'outer, je propose de réécrire plutôt:

If let Some((patern, maybe_check_url)) = proc_domains.on_demand_domains.iter().find(|(pattern, _)| pattern.matches(&candidate)) {

Pour éviter la double boucle et de devoir faire des `break 'outer`, je propose de réécrire plutôt: `If let Some((patern, maybe_check_url)) = proc_domains.on_demand_domains.iter().find(|(pattern, _)| pattern.matches(&candidate)) {`
quentin commented 2023-11-30 18:17:30 +00:00
Author
Owner
Copy link

Ah oui ça me semble bien !

Ah oui ça me semble bien !
src/cert_store.rs
@ -127,0 +198,4 @@
// if a check url is set, call it
// -- avoid DDoSing a backend
tokio::time::sleep(Duration::from_secs(2)).await;
lx commented 2023-11-30 17:18:34 +00:00
Owner
Copy link

Je pense que le délai peut être abaissé à 100ms, voir complètement supprimé, en tout cas dans un cas où c'est Garage le backend

Je pense que le délai peut être abaissé à 100ms, voir complètement supprimé, en tout cas dans un cas où c'est Garage le backend
quentin commented 2023-11-30 18:19:02 +00:00
Author
Owner
Copy link

J'aime bien quand même l'idée de garder un délai, ça nous évitera de DoS un truc si un bot venait à spam d'une manière imprévue tricot. Je préfère que la partie control plane de tricot soit ralentie que on propage la charge en arrière. Meme si je reconnais que c'est quand meme bien primitif comme méthode de controle. Hmm, du coup 100ms ça me parait être un bon compromis.

J'aime bien quand même l'idée de garder un délai, ça nous évitera de DoS un truc si un bot venait à spam d'une manière imprévue tricot. Je préfère que la partie control plane de tricot soit ralentie que on propage la charge en arrière. Meme si je reconnais que c'est quand meme bien primitif comme méthode de controle. Hmm, du coup 100ms ça me parait être un bon compromis.
lx commented 2023-11-30 18:54:34 +00:00
Owner
Copy link

Mon hypothèse c'était que même si on se prend un DoS, vu que les requêtes vers le back-end garage sont pas faites en parallèle mais l'une après l'autre, même sans délai ça fera jamais tomber garage en fait, c'est rien du tout par rapport à ce qu'il peut gérer. Du coup la proposition d'enlever le délai ça permettait juste de pas avoir un délai dans le happy path qui serait pas particulièrement utile ^^ Après 100ms de délai c'est vraiment pas grand chose donc ça me va aussi de le laisser

Mon hypothèse c'était que même si on se prend un DoS, vu que les requêtes vers le back-end garage sont pas faites en parallèle mais l'une après l'autre, même sans délai ça fera jamais tomber garage en fait, c'est rien du tout par rapport à ce qu'il peut gérer. Du coup la proposition d'enlever le délai ça permettait juste de pas avoir un délai dans le happy path qui serait pas particulièrement utile ^^ Après 100ms de délai c'est vraiment pas grand chose donc ça me va aussi de le laisser
quentin commented 2023-11-30 18:19:29 +00:00
Author
Owner
Copy link

J'ai bien vu tes remarques, j'essaie de les corriger plus tard, besoin de faire une pause là ^^
Merci pour tes retours !

J'ai bien vu tes remarques, j'essaie de les corriger plus tard, besoin de faire une pause là ^^ Merci pour tes retours !
👍 1
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/tricot#11
No description provided.
Delete branch "domain-check"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?