-
Notifications
You must be signed in to change notification settings - Fork 176
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
attempts to probe advertise address on startup to ensure the SANS is correct #2723
Changes from 6 commits
94ee830
2527896
bb04bbe
af644ec
bd3be1b
52520f1
834cf09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -880,6 +880,42 @@ func LoadConfigWithOptions(path string, loadIdentity bool) (*Config, error) { | |
return nil, err | ||
} | ||
|
||
if loadIdentity { | ||
var errs []error | ||
for _, c := range cfg.Link.Listeners { | ||
a := c["advertise"] | ||
if a != nil { | ||
// should start with tls: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Should" or "can?" My naive observation has been that the |
||
addy := strings.TrimPrefix(a.(string), "tls:") | ||
addy = strings.Split(addy, ":")[0] | ||
e := cfg.Id.ValidFor(addy) | ||
if e != nil { | ||
errs = append(errs, fmt.Errorf("invalid link.listeners.advertise: %s, error: %v", a.(string), e)) | ||
} | ||
} | ||
} | ||
|
||
for _, c := range cfg.Listeners { | ||
opts := c.options["options"] | ||
if opts != nil { | ||
optOpts := opts.(map[interface{}]interface{}) | ||
o := optOpts["advertise"] | ||
if o != nil { | ||
// should start with tls: | ||
addy := strings.TrimPrefix(o.(string), "tls:") | ||
addy = strings.Split(addy, ":")[0] | ||
e := cfg.Id.ValidFor(addy) | ||
if e != nil { | ||
errs = append(errs, fmt.Errorf("invalid listeners.binding.advertise: %s, error: %v", o.(string), e)) | ||
} | ||
} | ||
} | ||
} | ||
|
||
if len(errs) > 0 { | ||
pfxlog.Logger().Fatalf("one or more advertise addresses are invalid: %v", errs) | ||
} | ||
} | ||
return cfg, nil | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be removed.
This will fail if the advertised address goes to an xweb server that uses an alternate identity.
Since xweb now verifies its public address is represented by a SAN on its identity, we can rely on that to make sure the xweb config is sane.
There is already code to check that the advertise address is present on an xweb server that 1) shares the same address and 2) has the edge-api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about routers or controllers connecting? those aren't xweb, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but - routers/controllers are always specified outside of this config file? I just didn't know (still don't) if this address is used outside of xweb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is only for controllers xweb powered APIs. So it won't affect anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point about the ctrl listener using the root identity though. Which is a reasonable check.