Skip to content

Add --ip-address flag - #2

Closed
jsha wants to merge 4 commits into
ip-addrfrom
ip-addr-flag
Closed

jsha wants to merge 4 commits into
ip-addrfrom
ip-addr-flag

Conversation

@jsha

@jsha jsha commented Nov 6, 2025

Copy link
Copy Markdown
Owner

No description provided.

@bmw

bmw commented Nov 14, 2025

Copy link
Copy Markdown

while not a problem for certbot#10468, i think there may be one more area of certbot that needs some work before it's really ready for your code here. that is the code in certbot/src/certbot/_internal/auth_handler.py

since it's always been true for certbot until now, it repeatedly makes assumptions that the value of identifiers in the authorizations are domain names. this is a problem in logging output, the construction of AnnotatedChallenges, and even creates problems across the plugin API where calls to get_chall_pref made here will be passed IP addresses as part of the domain parameter

i unfortunately don't immediately have an idea on how to deal with these different problems, but i wanted to flag it for you in case you had time and interest to think about it

jsha and others added 3 commits November 20, 2025 14:03
In certbot#10478 we added a `san.SAN` class, with two subclasses `san.DNSName`
and `san.IPAddress`, so we can carry type information about identifiers
through the Certbot code. This PR plumbs through those types in most
Certbot-internal code. Note that this does not change the `acme` module,
which uses `messages.Identifier`. It also tries to leave alone the code
paths into plugins.

This does not add a CLI flag to request an IP address certificate. That
will be in a followup PR.

Part of certbot#10346
…0490)

this pr is in response to https://words.filippo.io/compromise-survey/.
ohemorange and i read this late on a friday to (speaking for myself at
least) much panic as it has some very strong words to say about the
github actions trigger pull_request_target which we use. looking into
the issue more, i also found that the popular static analysis tool
[zizmor](https://github.com/zizmorcore/zizmor) flags any github actions
workflow that uses the pull_request_target trigger with the message:

```
error[dangerous-triggers]: use of fundamentally insecure workflow trigger
pull_request_target is almost always used insecurely
```

this only added to my concern

the general problem with pull_request_target is that it runs with
additional privileges (e.g. potential write access, access to secrets)
in an environment containing values that can be set by an attacker.
these values include things such as references to the arbitrary code
contained in the triggering pr and pr titles which have been used to
perform shell injection attacks. not carefully treating these values
like the untrusted data it is while executing code in the privileged
environment given to pull_request_target has resulted in many supply
chain attacks

that's not to say that pull_request_target CAN'T be used securely.
zizmor even has [an
issue](zizmorcore/zizmor#1168) brainstorming
how to not warn about all uses of the trigger as some are clearly fine
and the only way to accomplish what the user wants. i'm going to argue
that our uses of the trigger are ok

looking through the links provided by filippo's blog and [zizmor's
docs](https://docs.zizmor.sh/audits/#dangerous-triggers), i think we can
break down attacks used against pull_request_target into roughly 2
categories:

1. shell injection: "Nx S1ingularity" and "Ultralytics" from filippo's
blog
2. checking out and running a PR's code: "Kong Ingress Controller" and
"Rspack" from filippo's blog and https://ptrpa.ws/nixpkgs-actions-abuse
from zizmor docs

i think none of our pull_request_target workflows have these problems.
none of them use a shell (the [zizmor
issue](zizmorcore/zizmor#1168) i linked
earlier suggests that any pull_request_target workflow that uses a run
block should always be flagged as insecure). instead, our workflows just
call action-mattermost-notify which can be [pretty easily
audited](https://github.com/mattermost/action-mattermost-notify/blob/2.0.0/src/main.js)
(as all the other files in the repo are boilerplate). passing possible
attacker controlled values directly to an action written in another
language is one of the approaches for mitigating script injection
[recommended by
github](https://docs.github.com/en/actions/reference/security/secure-use#use-an-action-instead-of-an-inline-script).
our workflows also do not check out the triggering pr's code

despite all that, i took this opportunity to cleanup and harden things a
bit. i reduced the permissions for each workflow and confirmed they each
still work on my fork. i also pinned the mattermost action to an exact
version and added some inline documentation

with these changes, our github workflows trigger few to no
warnings/errors when checked with zizmor,
[octoscan](https://github.com/synacktiv/octoscan), and [openssf
scorecard](https://github.com/ossf/scorecard)

if this pr is approved, i'll make similar changes to our josepy repo
@jsha jsha closed this Nov 21, 2025
ohemorange added a commit to certbot/certbot that referenced this pull request Dec 5, 2025
This field is optional to maintain backwards compatibility. Note that
`AnnotatedChallenge` inherits from `jose.ImmutableMap`, which has a
[check in
__init__](https://github.com/certbot/josepy/blob/4b747476703fe4fff1aaccd76ebe570698bbf4f0/src/josepy/util.py#L125-L131)
that all slots are provided. That check would not allow us to do a
backwards-compatible addition, so I implemented an `__init__` for each
of these subclasses that fills the fields without calling the parent
`__init__`, and so doesn't hit an error when `identifier` is absent.

I chose to use `acme.messages.Identifier` rather than
`certbot._internal.san.SAN` here because these are wrapped ACME types,
so they should use the ACME representation. Also, `AnnotatedChallenge`
is passed to plugins, so we need to pass a type that the plugins can
understand.

Additionally, `domain` is marked as deprecated.

Part of #10346

/cc @bmw, who noticed the issue with `AnnotatedChallenge`
[here](#10468 (comment))
and provided additional feedback
[here](jsha#2 (comment)).
Note that there's still some work to do to finish excising `domain`
assumptions from this portion of the code.

---------

Co-authored-by: ohemorange <[email protected]>
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.

2 participants