Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replacer: Implement file.* global replacements #5463

Merged
merged 7 commits into from
Apr 24, 2024
Merged

Conversation

francislavoie
Copy link
Member

Closes #5374

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks Francis -- let's document this as an experimental feature. I'd also like to see some confirmation from someone that this is in fact useful for them and what they need. And yes, I agree we can lean on the kernel to cache files in hot paths and that will probably get us 99% of the way there in terms of peak performance. 👍

Thanks again!

replacer.go Outdated Show resolved Hide resolved
@francislavoie
Copy link
Member Author

The Docker secrets argument is compelling enough for me. It's a legitimate trend.

@francislavoie francislavoie enabled auto-merge (squash) March 27, 2023 19:34
@francislavoie francislavoie disabled auto-merge March 27, 2023 19:36
@mholt
Copy link
Member

mholt commented Mar 27, 2023

Ok -- if we don't hear from anyone in a couple days, I'd say go ahead and merge it. Thank you!

@polarathene
Copy link

The Docker secrets argument is compelling enough for me. It's a legitimate trend.

But often a misunderstood one since it's not at parity to the swarm feature, it's just a volume mount under the hood without Docker Swarm.

However I think it might work properly with Podman and Kubernetes equivalents. Even with plain Docker, so long as the user is aware none of the Docker Swarm features apply, they can manually adjust ownership/permissions for the file before mounting it if needed.

@iliana
Copy link
Contributor

iliana commented May 11, 2023

Wanted to reach out and mention that I applied this PR to a v2.6.4 build and it works well for me!

One confusing thing is that this style of matcher didn't work for me:

@valid {
  method POST
  path {file./path/to/secret}
}

but this works:

@valid {
  method POST
  vars {path} {file./path/to/secret}
}

but I'm unsure whether that's expected.

@francislavoie
Copy link
Member Author

I don't understand what you're trying to do with that matcher. Please elaborate with more specifics. What does the request look like, and what's in the file?

@iliana
Copy link
Contributor

iliana commented May 11, 2023

What I am doing is configuring a POST webhook with a secret parameter in the URL, e.g. /webhook/WpFohBGulVjWHE2v3MP1tHB9ztcPAiD9PBNWF4HtvNQ. Because this config is built by a public repo, and the third-party system that is going to hit this webhook doesn't provide any way of authenticating requests, I am adding the long secret hash to help prevent guessing the URL.

The request I am matching is (using the above example) POST https://example.com/webhook/WpFohBGulVjWHE2v3MP1tHB9ztcPAiD9PBNWF4HtvNQ, and the section for this domain in my working Caddyfile looks like:

example.com {
        bind

        log {
                output file /var/log/caddy/access-example.com.log
        }

        encode zstd gzip
        tls {
                on_demand
        }

        route {
                @webhook {
                        method POST
                        vars {path} {file./run/agenix/webhook-path}
                }

                reverse_proxy @webhook unix//run/fcgiwrap.sock {
                        transport fastcgi {
                                env SCRIPT_FILENAME /nix/store/y2aawd2bjw9dzp1g4p5pjawn8786n7kn-pkgf/bin/pkgf
                                env WEBHOOK_SECRET {file./run/agenix/webhook-secret}
                        }
                }

                respond /yo "yo"

                error 404
        }
}

/run/agenix/webhook-path contains exactly /webhook/WpFohBGulVjWHE2v3MP1tHB9ztcPAiD9PBNWF4HtvNQ (no whitespace). With this configuration, POST requests to that path get proxied to fcgiwrap and Caddy responds with 200.

I tried this matcher first, however:

...
                @webhook {
                        method POST
                        path {file./run/agenix/webhook-path}
                }
...

and given the same request, Caddy responds with 404. This is unexpected behavior to me, but I am unsure if the path matcher would otherwise correctly handle this for some other replacer as I've never tried it.

@znkr
Copy link

znkr commented May 12, 2023

I'd also like to see some confirmation from someone that this is in fact useful for them and what they need

I found this PR while trying to understand how to best use docker, podman, or kubernetes secrets in a Caddyfile. That turned out to be quite the pain. Having a way to read secrets from a file directly would definitely help.

Podman and Kubernetes can add a secret into an environment variable, but that's not recommended as environment variables tend to leak.

@mholt
Copy link
Member

mholt commented May 15, 2023

Francis and I talked about this in Slack and while we agree there are use cases for this, we want to be sure we're careful since this currently is not jailed to the site root, and we're not always confident that placeholders come from trusted sources (though they should). For example, templates can invoke placeholders, and we're not sure that templates are all always trusted (i.e. if a site allows users to customize templates).

I might defer this a little later until we can figure out a safe mechanism for this.

@mholt mholt modified the milestones: v2.7.0, 2.x May 15, 2023
@mholt mholt added discussion 💬 The right solution needs to be found do not merge ⛔ Not ready yet! labels May 15, 2023
@francislavoie francislavoie force-pushed the file-placeholder branch 2 times, most recently from d8cd857 to 6153304 Compare October 14, 2023 22:29
@francislavoie
Copy link
Member Author

francislavoie commented Oct 14, 2023

Took me a while to think about a good way to make it safe. I think a "good enough" approach is to provide a way for specific modules to opt-out of the file placeholders.

The simplest way I could think of doing it is by prefixing the providers with one that rejects file placeholders before it reaches the existing one. I wanted to filter out the fileReplacements function, but apparently Go doesn't allow comparing functions (which I'm totally used to doing in languages like JS and PHP, so I found that surprising).

This still needs some more tests before merging (edit: added tests), but I think it might be an improvement in terms of safety. I'm not sure if there's other modules that should also opt-out of the placeholder, we'll need to review that.

@mohammed90
Copy link
Member

apparently Go doesn't allow comparing functions

I don't know what's the design you had in mind, but maybe you can work around needing to compare functions by interface-asserting into single-method interfaces and branch that way?

@francislavoie francislavoie modified the milestones: 2.x, v2.8.0 Feb 17, 2024
@francislavoie francislavoie removed do not merge ⛔ Not ready yet! discussion 💬 The right solution needs to be found labels Feb 18, 2024
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

One minor change since I'm not 100% committed to this feature/approach, but we can give it a try. :) Thanks

// WithoutFile returns a copy of the current Replacer
// without support for the {file.*} placeholder, which
// may be unsafe in some contexts.
func (r *Replacer) WithoutFile() *Replacer {
Copy link
Member

Choose a reason for hiding this comment

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

Mark this as Experimental. I'm still nervous 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. But I think it's less-so this function needing to be experimental, cause this is actually the opt-out.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. I mostly am just worried about compatibility promises in the very off-chance this is used externally 😆

@francislavoie francislavoie merged commit 7979739 into master Apr 24, 2024
25 checks passed
@francislavoie francislavoie deleted the file-placeholder branch April 24, 2024 20:26
@lorenzleutgeb
Copy link

lorenzleutgeb commented Apr 24, 2024

Looking forward to use this feature! Thanks! I tried to reconstruct a Caddyfile example from this to understand how I would use such a replacement in my own configuration, but it's a little too dense for a user to see right away. I hope you'll find the time to add an example to the docs somewhere, that would be great!

@francislavoie
Copy link
Member Author

francislavoie commented Apr 24, 2024

It's just this: {file./path/to/some/file}. Put that in your config wherever you need to read a config value from a file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support loading placeholder content from files (for systemd credentials or docker secrets)
7 participants