TheArchive/zitadel
1
0
Fork 0
mirror of https://github.com/zitadel/zitadel.git synced 2025-08-11 15:49:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(actions): handle empty deny list correctly (#9753)

Browse Source 
<!--
Please inform yourself about the contribution guidelines on submitting a
PR here:
https://github.com/zitadel/zitadel/blob/main/CONTRIBUTING.md#submit-a-pull-request-pr.
Take note of how PR/commit titles should be written and replace the
template texts in the sections below. Don't remove any of the sections.
It is important that the commit history clearly shows what is changed
and why.
Important: By submitting a contribution you agree to the terms from our
Licensing Policy as described here:
https://github.com/zitadel/zitadel/blob/main/LICENSING.md#community-contributions.
-->

# Which Problems Are Solved

A customer reached out that after an upgrade, actions would always fail
with the error "host is denied" when calling an external API.
This is due to a security fix
(https://github.com/zitadel/zitadel/security/advisories/GHSA-6cf5-w9h3-4rqv),
where a DNS lookup was added to check whether the host name resolves to
a denied IP or subnet.
If the lookup fails due to the internal DNS setup, the action fails as
well. Additionally, the lookup was also performed when the deny list was
empty.

# How the Problems Are Solved

- Prevent DNS lookup when deny list is empty
- Properly initiate deny list and prevent empty entries

# Additional Changes

- Log the reason for blocked address (domain, IP, subnet)

# Additional Context

- reported by a customer
- needs backport to 2.70.x, 2.71.x and 3.0.0 rc

(cherry picked from commit 4ffd4ef381)
This commit is contained in:
Livio Spring
2025-04-25 09:12:42 +02:00
parent fcc2cb170d
commit cd19b264b3
4 changed files with 65 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
cmd/start/config.go
View File

@@ -128,7 +128,9 @@ func MustNewConfig(v *viper.Viper) *Config {
logging.OnError(err).Fatal("unable to set profiler")
id.Configure(config.Machine)
actions.SetHTTPConfig(&config.Actions.HTTP)
if config.Actions != nil {
actions.SetHTTPConfig(&config.Actions.HTTP)
}
// Copy the global role permissions mappings to the instance until we allow instance-level configuration over the API.
config.DefaultInstance.RolePermissionMappings = config.InternalAuthZ.RolePermissionMappings

20
internal/actions/http_module.go
View File

@@ -176,16 +176,16 @@ type transport struct {
}
func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) {
if httpConfig == nil {
if httpConfig == nil || len(httpConfig.DenyList) == 0 {
return http.DefaultTransport.RoundTrip(req)
}
if t.isHostBlocked(httpConfig.DenyList, req.URL) {
return nil, zerrors.ThrowInvalidArgument(nil, "ACTIO-N72d0", "host is denied")
if err := t.isHostBlocked(httpConfig.DenyList, req.URL); err != nil {
return nil, zerrors.ThrowInvalidArgument(err, "ACTIO-N72d0", "host is denied")
}
return http.DefaultTransport.RoundTrip(req)
}
func (t *transport) isHostBlocked(denyList []AddressChecker, address *url.URL) bool {
func (t *transport) isHostBlocked(denyList []AddressChecker, address *url.URL) error {
host := address.Hostname()
ip := net.ParseIP(host)
ips := []net.IP{ip}
@@ -194,17 +194,17 @@ func (t *transport) isHostBlocked(denyList []AddressChecker, address *url.URL) b
var err error
ips, err = t.lookup(host)
if err != nil {
return true
return zerrors.ThrowInternal(err, "ACTIO-4m9s2", "lookup failed")
}
}
for _, blocked := range denyList {
if blocked.Matches(ips, host) {
return true
for _, denied := range denyList {
if err := denied.IsDenied(ips, host); err != nil {
return err
}
}
return false
return nil
}
type AddressChecker interface {
Matches([]net.IP, string) bool
IsDenied([]net.IP, string) error
}

35
internal/actions/http_module_config.go
View File

@@ -1,6 +1,8 @@
package actions
import (
"errors"
"fmt"
"net"
"reflect"
"strings"
@@ -60,6 +62,9 @@ func HTTPConfigDecodeHook(from, to reflect.Value) (interface{}, error) {
}
func NewHostChecker(entry string) (AddressChecker, error) {
if entry == "" {
return nil, nil
}
_, network, err := net.ParseCIDR(entry)
if err == nil {
return &HostChecker{Net: network}, nil
@@ -76,19 +81,39 @@ type HostChecker struct {
Domain string
}
func (c *HostChecker) Matches(ips []net.IP, address string) bool {
type AddressDeniedError struct {
deniedBy string
}
func NewAddressDeniedError(deniedBy string) *AddressDeniedError {
return &AddressDeniedError{deniedBy: deniedBy}
}
func (e *AddressDeniedError) Error() string {
return fmt.Sprintf("address is denied by '%s'", e.deniedBy)
}
func (e *AddressDeniedError) Is(target error) bool {
var addressDeniedErr *AddressDeniedError
if !errors.As(target, &addressDeniedErr) {
return false
}
return e.deniedBy == addressDeniedErr.deniedBy
}
func (c *HostChecker) IsDenied(ips []net.IP, address string) error {
// if the address matches the domain, no additional checks as needed
if c.Domain == address {
return true
return NewAddressDeniedError(c.Domain)
}
// otherwise we need to check on ips (incl. the resolved ips of the host)
for _, ip := range ips {
if c.Net != nil && c.Net.Contains(ip) {
return true
return NewAddressDeniedError(c.Net.String())
}
if c.IP != nil && c.IP.Equal(ip) {
return true
return NewAddressDeniedError(c.IP.String())
}
}
return false
return nil
}

31
internal/actions/http_module_test.go
View File

@@ -3,6 +3,7 @@ package actions
import (
"bytes"
"context"
"errors"
"io"
"net"
"net/http"
@@ -11,6 +12,7 @@ import (
"testing"
"github.com/dop251/goja"
"github.com/stretchr/testify/assert"
"github.com/zitadel/zitadel/internal/logstore"
"github.com/zitadel/zitadel/internal/logstore/record"
@@ -34,21 +36,21 @@ func Test_isHostBlocked(t *testing.T) {
name string
fields fields
args args
want bool
want error
}{
{
name: "in range",
args: args{
address: mustNewURL(t, "https://192.168.5.4/hodor"),
},
want: true,
want: NewAddressDeniedError("192.168.5.0/24"),
},
{
name: "exact ip",
args: args{
address: mustNewURL(t, "http://127.0.0.1:8080/hodor"),
},
want: true,
want: NewAddressDeniedError("127.0.0.1"),
},
{
name: "address match",
@@ -60,7 +62,7 @@ func Test_isHostBlocked(t *testing.T) {
args: args{
address: mustNewURL(t, "https://test.com:42/hodor"),
},
want: true,
want: NewAddressDeniedError("test.com"),
},
{
name: "address not match",
@@ -72,7 +74,7 @@ func Test_isHostBlocked(t *testing.T) {
args: args{
address: mustNewURL(t, "https://test2.com/hodor"),
},
want: false,
want: nil,
},
{
name: "looked up ip matches",
@@ -84,7 +86,19 @@ func Test_isHostBlocked(t *testing.T) {
args: args{
address: mustNewURL(t, "https://test2.com/hodor"),
},
want: true,
want: NewAddressDeniedError("127.0.0.1"),
},
{
name: "looked up failure",
fields: fields{
lookup: func(host string) ([]net.IP, error) {
return nil, errors.New("some error")
},
},
args: args{
address: mustNewURL(t, "https://test2.com/hodor"),
},
want: zerrors.ThrowInternal(nil, "ACTIO-4m9s2", "lookup failed"),
},
}
for _, tt := range tests {
@@ -92,9 +106,8 @@ func Test_isHostBlocked(t *testing.T) {
trans := &transport{
lookup: tt.fields.lookup,
}
if got := trans.isHostBlocked(denyList, tt.args.address); got != tt.want {
t.Errorf("isHostBlocked() = %v, want %v", got, tt.want)
}
got := trans.isHostBlocked(denyList, tt.args.address)
assert.ErrorIs(t, got, tt.want)
})
}
}