core/config: remove support for base64 encoded certificates #4718
Conversation
calebdoxsey
added
the
breaking
There was a problem hiding this comment.
for follow-up discussion: https://github.com/pomerium/internal/issues/1582
| cfp.KeyFile = base64.StdEncoding.EncodeToString(c.KeyBytes) | ||
|
|
||
| cert, err := cryptutil.ParsePEMCertificateFromBase64(cfp.CertFile) | ||
| cert, err := cryptutil.ParsePEMCertificate(c.GetCertBytes()) |
There was a problem hiding this comment.
what do you think about dropping validation in this function, and making it just concatenate the data and that's all.
this function is called from ApplySettings which is not returning any error
Line 1411 in ffca3b3
which, in turn, is called from config_source:
pomerium/internal/databroker/config_source.go
Lines 174 to 178 in ffca3b3
which merges data from external clients, which are supposed to perform config validation on their side?
plus there's additional cert parsing and erroring in Options.GetCertificates and Options.GetX509Certificates
There was a problem hiding this comment.
So to clarify, we would like to rollback the requirements from https://github.com/pomerium/internal/issues/449 ?
We no longer want to enforce the call to certsIndex.OverlapsWithExistingCertificate(cert) to prevent console / ingress controller certificates from overriding core certificates?
There was a problem hiding this comment.
you are right. let's keep it then.
I can add skipping this check in case validation is disabled (certsIndex == nil) in a follow up pr
pomerium/internal/databroker/config_source.go
Lines 167 to 169 in 205aa8c
config/config_source_test.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestFileWatcherSource(t *testing.T) { |
* core/config: update file watcher source to handle base64 encoded certificates * fix data race * core/config: only allow files in certificates * remove test * re-add test
core/config: remove support for base64 encoded certificates (#4718) * core/config: update file watcher source to handle base64 encoded certificates * fix data race * core/config: only allow files in certificates * remove test * re-add test Co-authored-by: Caleb Doxsey <cdoxsey@pomerium.com>
Summary
Remove support for base64 encoded certificates in the
certificatesfield (confusingly namedCertificateFilesin the optinos). This is a breaking change.Related issues
Fixes https://github.com/pomerium/internal/issues/1580
Checklist
improvement/bug/ etc)