Skip to content

Commit a8bb68e

Browse files
committed
Don't panic in SSL code
This change removes panics from SSL code. This is a part of the ongoing process of removing panics from lib/pq code. Refs #681.
1 parent 614cb79 commit a8bb68e

File tree

4 files changed

+63
-35
lines changed

4 files changed

+63
-35
lines changed

conn.go

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -340,15 +340,19 @@ func DialOpen(d Dialer, name string) (_ driver.Conn, err error) {
340340
return nil, err
341341
}
342342

343-
// cn.ssl and cn.startup panic on error. Make sure we don't leak cn.c.
343+
err = cn.ssl(o)
344+
if err != nil {
345+
return nil, err
346+
}
347+
348+
// cn.startup panics on error. Make sure we don't leak cn.c.
344349
panicking := true
345350
defer func() {
346351
if panicking {
347352
cn.c.Close()
348353
}
349354
}()
350355

351-
cn.ssl(o)
352356
cn.buf = bufio.NewReader(cn.c)
353357
cn.startup(o)
354358

@@ -1029,30 +1033,35 @@ func (cn *conn) recv1() (t byte, r *readBuf) {
10291033
return t, r
10301034
}
10311035

1032-
func (cn *conn) ssl(o values) {
1033-
upgrade := ssl(o)
1036+
func (cn *conn) ssl(o values) error {
1037+
upgrade, err := ssl(o)
1038+
if err != nil {
1039+
return err
1040+
}
1041+
10341042
if upgrade == nil {
10351043
// Nothing to do
1036-
return
1044+
return nil
10371045
}
10381046

10391047
w := cn.writeBuf(0)
10401048
w.int32(80877103)
1041-
if err := cn.sendStartupPacket(w); err != nil {
1042-
panic(err)
1049+
if err = cn.sendStartupPacket(w); err != nil {
1050+
return err
10431051
}
10441052

10451053
b := cn.scratch[:1]
1046-
_, err := io.ReadFull(cn.c, b)
1054+
_, err = io.ReadFull(cn.c, b)
10471055
if err != nil {
1048-
panic(err)
1056+
return err
10491057
}
10501058

10511059
if b[0] != 'S' {
1052-
panic(ErrSSLNotSupported)
1060+
return ErrSSLNotSupported
10531061
}
10541062

1055-
cn.c = upgrade(cn.c)
1063+
cn.c, err = upgrade(cn.c)
1064+
return err
10561065
}
10571066

10581067
// isDriverSetting returns true iff a setting is purely for configuring the

conn_go18.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,10 @@ func (cn *conn) cancel() error {
108108
can := conn{
109109
c: c,
110110
}
111-
can.ssl(cn.opts)
111+
err = can.ssl(cn.opts)
112+
if err != nil {
113+
return err
114+
}
112115

113116
w := can.writeBuf(0)
114117
w.int32(80877102) // cancel request code

error.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,11 @@ func errorf(s string, args ...interface{}) {
460460
panic(fmt.Errorf("pq: %s", fmt.Sprintf(s, args...)))
461461
}
462462

463+
// TODO(ainar-g) Rename to errorf after removing panics.
464+
func fmterrorf(s string, args ...interface{}) error {
465+
return fmt.Errorf("pq: %s", fmt.Sprintf(s, args...))
466+
}
467+
463468
func errRecoverNoErrBadConn(err *error) {
464469
e := recover()
465470
if e == nil {

ssl.go

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212

1313
// ssl generates a function to upgrade a net.Conn based on the "sslmode" and
1414
// related settings. The function is nil when no upgrade should take place.
15-
func ssl(o values) func(net.Conn) net.Conn {
15+
func ssl(o values) (func(net.Conn) (net.Conn, error), error) {
1616
verifyCaOnly := false
1717
tlsConf := tls.Config{}
1818
switch mode := o["sslmode"]; mode {
@@ -45,29 +45,38 @@ func ssl(o values) func(net.Conn) net.Conn {
4545
case "verify-full":
4646
tlsConf.ServerName = o["host"]
4747
case "disable":
48-
return nil
48+
return nil, nil
4949
default:
50-
errorf(`unsupported sslmode %q; only "require" (default), "verify-full", "verify-ca", and "disable" supported`, mode)
50+
return nil, fmterrorf(`unsupported sslmode %q; only "require" (default), "verify-full", "verify-ca", and "disable" supported`, mode)
5151
}
5252

53-
sslClientCertificates(&tlsConf, o)
54-
sslCertificateAuthority(&tlsConf, o)
53+
err := sslClientCertificates(&tlsConf, o)
54+
if err != nil {
55+
return nil, err
56+
}
57+
err = sslCertificateAuthority(&tlsConf, o)
58+
if err != nil {
59+
return nil, err
60+
}
5561
sslRenegotiation(&tlsConf)
5662

57-
return func(conn net.Conn) net.Conn {
63+
return func(conn net.Conn) (net.Conn, error) {
5864
client := tls.Client(conn, &tlsConf)
5965
if verifyCaOnly {
60-
sslVerifyCertificateAuthority(client, &tlsConf)
66+
err := sslVerifyCertificateAuthority(client, &tlsConf)
67+
if err != nil {
68+
return nil, err
69+
}
6170
}
62-
return client
63-
}
71+
return client, nil
72+
}, nil
6473
}
6574

6675
// sslClientCertificates adds the certificate specified in the "sslcert" and
6776
// "sslkey" settings, or if they aren't set, from the .postgresql directory
6877
// in the user's home directory. The configured files must exist and have
6978
// the correct permissions.
70-
func sslClientCertificates(tlsConf *tls.Config, o values) {
79+
func sslClientCertificates(tlsConf *tls.Config, o values) error {
7180
// user.Current() might fail when cross-compiling. We have to ignore the
7281
// error and continue without home directory defaults, since we wouldn't
7382
// know from where to load them.
@@ -82,13 +91,13 @@ func sslClientCertificates(tlsConf *tls.Config, o values) {
8291
}
8392
// https://github.com/postgres/postgres/blob/REL9_6_2/src/interfaces/libpq/fe-secure-openssl.c#L1045
8493
if len(sslcert) == 0 {
85-
return
94+
return nil
8695
}
8796
// https://github.com/postgres/postgres/blob/REL9_6_2/src/interfaces/libpq/fe-secure-openssl.c#L1050:L1054
8897
if _, err := os.Stat(sslcert); os.IsNotExist(err) {
89-
return
98+
return nil
9099
} else if err != nil {
91-
panic(err)
100+
return err
92101
}
93102

94103
// In libpq, the ssl key is only loaded if the setting is not blank.
@@ -101,19 +110,21 @@ func sslClientCertificates(tlsConf *tls.Config, o values) {
101110

102111
if len(sslkey) > 0 {
103112
if err := sslKeyPermissions(sslkey); err != nil {
104-
panic(err)
113+
return err
105114
}
106115
}
107116

108117
cert, err := tls.LoadX509KeyPair(sslcert, sslkey)
109118
if err != nil {
110-
panic(err)
119+
return err
111120
}
121+
112122
tlsConf.Certificates = []tls.Certificate{cert}
123+
return nil
113124
}
114125

115126
// sslCertificateAuthority adds the RootCA specified in the "sslrootcert" setting.
116-
func sslCertificateAuthority(tlsConf *tls.Config, o values) {
127+
func sslCertificateAuthority(tlsConf *tls.Config, o values) error {
117128
// In libpq, the root certificate is only loaded if the setting is not blank.
118129
//
119130
// https://github.com/postgres/postgres/blob/REL9_6_2/src/interfaces/libpq/fe-secure-openssl.c#L950-L951
@@ -122,22 +133,24 @@ func sslCertificateAuthority(tlsConf *tls.Config, o values) {
122133

123134
cert, err := ioutil.ReadFile(sslrootcert)
124135
if err != nil {
125-
panic(err)
136+
return err
126137
}
127138

128139
if !tlsConf.RootCAs.AppendCertsFromPEM(cert) {
129-
errorf("couldn't parse pem in sslrootcert")
140+
return fmterrorf("couldn't parse pem in sslrootcert")
130141
}
131142
}
143+
144+
return nil
132145
}
133146

134147
// sslVerifyCertificateAuthority carries out a TLS handshake to the server and
135148
// verifies the presented certificate against the CA, i.e. the one specified in
136149
// sslrootcert or the system CA if sslrootcert was not specified.
137-
func sslVerifyCertificateAuthority(client *tls.Conn, tlsConf *tls.Config) {
150+
func sslVerifyCertificateAuthority(client *tls.Conn, tlsConf *tls.Config) error {
138151
err := client.Handshake()
139152
if err != nil {
140-
panic(err)
153+
return err
141154
}
142155
certs := client.ConnectionState().PeerCertificates
143156
opts := x509.VerifyOptions{
@@ -152,7 +165,5 @@ func sslVerifyCertificateAuthority(client *tls.Conn, tlsConf *tls.Config) {
152165
opts.Intermediates.AddCert(cert)
153166
}
154167
_, err = certs[0].Verify(opts)
155-
if err != nil {
156-
panic(err)
157-
}
168+
return err
158169
}

0 commit comments

Comments
 (0)