Skip to content

Commit a8b9564

Browse files
vonzshikvonzshik
authored
Fix synchronous GSS session encryption and enable it by default (#6324)
Followup to #2957
1 parent 302af43 commit a8b9564

File tree

4 files changed

+109
-69
lines changed

4 files changed

+109
-69
lines changed

‎src/Npgsql/Internal/NpgsqlConnector.Auth.cs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,11 @@ internal async ValueTask AuthenticateGSS(bool async, CancellationToken cancellat
336336

337337
using var authContext = new NegotiateAuthentication(clientOptions);
338338
var data = authContext.GetOutgoingBlob(ReadOnlySpan<byte>.Empty, out var statusCode)!;
339-
Debug.Assert(statusCode == NegotiateAuthenticationStatusCode.ContinueNeeded);
339+
if (statusCode != NegotiateAuthenticationStatusCode.ContinueNeeded)
340+
{
341+
// Unable to retrieve credentials or some other issue
342+
throw new NpgsqlException($"Unable to authenticate with GSS: received {statusCode} instead of the expected ContinueNeeded");
343+
}
340344
await WritePassword(data, 0, data.Length, async, cancellationToken).ConfigureAwait(false);
341345
await Flush(async, cancellationToken).ConfigureAwait(false);
342346
while (true)

‎src/Npgsql/Internal/NpgsqlConnector.cs‎

Lines changed: 102 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -596,17 +596,27 @@ static async Task OpenCore(
596596
bool async,
597597
CancellationToken cancellationToken)
598598
{
599-
await conn.RawOpen(sslMode, gssEncMode, timeout, async, cancellationToken).ConfigureAwait(false);
600-
timeout.CheckAndApply(conn);
601-
conn.WriteStartupMessage(username);
602-
await conn.Flush(async, cancellationToken).ConfigureAwait(false);
599+
// If we fail to connect to the socket, there is no reason to retry even if SslMode/GssEncryption allows it
600+
await conn.RawOpen(timeout, async, cancellationToken).ConfigureAwait(false);
603601

604-
using var cancellationRegistration = conn.StartCancellableOperation(cancellationToken, attemptPgCancellation: false);
605602
try
606603
{
604+
await conn.SetupEncryption(sslMode, gssEncMode, timeout, async, cancellationToken).ConfigureAwait(false);
605+
timeout.CheckAndApply(conn);
606+
conn.WriteStartupMessage(username);
607+
await conn.Flush(async, cancellationToken).ConfigureAwait(false);
608+
609+
using var cancellationRegistration = conn.StartCancellableOperation(cancellationToken, attemptPgCancellation: false);
607610
await conn.Authenticate(username, timeout, async, cancellationToken).ConfigureAwait(false);
608611
}
612+
// We handle any exception here because on Windows while receiving a response from Postgres
613+
// We might hit connection reset, in which case the actual error will be lost
614+
// And we only read some IO error
615+
// In addition, this behavior mimics libpq, where it retries as long as GssEncryptionMode and SslMode allows it
609616
catch (Exception e) when
617+
// We might also get here OperationCancelledException/TimeoutException
618+
// But it's fine to fall down and retry because we'll immediately exit with the exact same exception
619+
//
610620
// Any error after trying with GSS encryption
611621
(gssEncMode == GssEncryptionMode.Prefer ||
612622
// Auth error with/without SSL
@@ -620,9 +630,6 @@ static async Task OpenCore(
620630
else
621631
sslMode = sslMode == SslMode.Prefer ? SslMode.Disable : SslMode.Require;
622632

623-
cancellationRegistration.Dispose();
624-
Debug.Assert(!conn.IsBroken);
625-
626633
conn.Cleanup();
627634

628635
// If Prefer was specified and we failed (with SSL), retry without SSL.
@@ -696,6 +703,8 @@ internal async ValueTask<GssEncryptionResult> GSSEncrypt(bool async, bool isRequ
696703
default:
697704
throw new NpgsqlException($"Received unknown response {response} for GSSEncRequest (expecting G or N)");
698705
case 'N':
706+
if (isRequired)
707+
throw new NpgsqlException("GGS encryption requested. No GSS encryption enabled connection from this host is configured.");
699708
return GssEncryptionResult.NegotiateFailure;
700709
case 'G':
701710
break;
@@ -907,7 +916,7 @@ async ValueTask<string> GetUsernameAsyncInternal()
907916
}
908917
}
909918

910-
async Task RawOpen(SslMode sslMode, GssEncryptionMode gssEncryptionMode, NpgsqlTimeout timeout, bool async, CancellationToken cancellationToken)
919+
async Task RawOpen(NpgsqlTimeout timeout, bool async, CancellationToken cancellationToken)
911920
{
912921
try
913922
{
@@ -939,59 +948,6 @@ async Task RawOpen(SslMode sslMode, GssEncryptionMode gssEncryptionMode, NpgsqlT
939948

940949
IsSslEncrypted = false;
941950
IsGssEncrypted = false;
942-
943-
var gssEncryptResult = await TryNegotiateGssEncryption(gssEncryptionMode, async, cancellationToken).ConfigureAwait(false);
944-
if (gssEncryptResult == GssEncryptionResult.Success)
945-
return;
946-
947-
timeout.CheckAndApply(this);
948-
949-
if (GetSslNegotiation(Settings) == SslNegotiation.Direct)
950-
{
951-
// We already check that in NpgsqlConnectionStringBuilder.PostProcessAndValidate, but since we also allow environment variables...
952-
if (Settings.SslMode is not SslMode.Require and not SslMode.VerifyCA and not SslMode.VerifyFull)
953-
throw new ArgumentException("SSL Mode has to be Require or higher to be used with direct SSL Negotiation");
954-
if (gssEncryptResult == GssEncryptionResult.NegotiateFailure)
955-
{
956-
// We can be here only if it's fallback from preferred (but failed) gss encryption
957-
// In this case, direct encryption isn't going to work anymore, so we throw a bogus exception to retry again without gss
958-
// Alternatively, we can instead just go with the usual route of writing SslRequest, ignoring direct ssl
959-
// But this is how libpq works
960-
Debug.Assert(gssEncryptionMode == GssEncryptionMode.Prefer);
961-
// The exception message doesn't matter since we're going to retry again
962-
throw new NpgsqlException();
963-
}
964-
965-
await DataSource.TransportSecurityHandler.NegotiateEncryption(async, this, sslMode, timeout, cancellationToken).ConfigureAwait(false);
966-
if (ReadBuffer.ReadBytesLeft > 0)
967-
throw new NpgsqlException("Additional unencrypted data received after SSL negotiation - this should never happen, and may be an indication of a man-in-the-middle attack.");
968-
}
969-
else if ((sslMode is SslMode.Prefer && DataSource.TransportSecurityHandler.SupportEncryption) ||
970-
sslMode is SslMode.Require or SslMode.VerifyCA or SslMode.VerifyFull)
971-
{
972-
WriteSslRequest();
973-
await Flush(async, cancellationToken).ConfigureAwait(false);
974-
975-
await ReadBuffer.Ensure(1, async).ConfigureAwait(false);
976-
var response = (char)ReadBuffer.ReadByte();
977-
timeout.CheckAndApply(this);
978-
979-
switch (response)
980-
{
981-
default:
982-
throw new NpgsqlException($"Received unknown response {response} for SSLRequest (expecting S or N)");
983-
case 'N':
984-
if (sslMode != SslMode.Prefer)
985-
throw new NpgsqlException("SSL connection requested. No SSL enabled connection from this host is configured.");
986-
break;
987-
case 'S':
988-
await DataSource.TransportSecurityHandler.NegotiateEncryption(async, this, sslMode, timeout, cancellationToken).ConfigureAwait(false);
989-
break;
990-
}
991-
992-
if (ReadBuffer.ReadBytesLeft > 0)
993-
throw new NpgsqlException("Additional unencrypted data received after SSL negotiation - this should never happen, and may be an indication of a man-in-the-middle attack.");
994-
}
995951
}
996952
catch
997953
{
@@ -1008,12 +964,80 @@ async Task RawOpen(SslMode sslMode, GssEncryptionMode gssEncryptionMode, NpgsqlT
1008964
}
1009965
}
1010966

967+
async Task SetupEncryption(SslMode sslMode, GssEncryptionMode gssEncryptionMode, NpgsqlTimeout timeout, bool async, CancellationToken cancellationToken)
968+
{
969+
var gssEncryptResult = await TryNegotiateGssEncryption(gssEncryptionMode, async, cancellationToken).ConfigureAwait(false);
970+
if (gssEncryptResult == GssEncryptionResult.Success)
971+
return;
972+
973+
// TryNegotiateGssEncryption should already throw a much more meaningful exception
974+
// if GSS encryption is required but for some reason we can't negotiate it.
975+
// But since we have to return a specific result instead of generic true/false
976+
// To make absolutely sure we didn't miss anything, recheck again
977+
if (gssEncryptionMode == GssEncryptionMode.Require)
978+
throw new NpgsqlException($"Unable to negotiate GSS encryption: {gssEncryptResult}");
979+
980+
timeout.CheckAndApply(this);
981+
982+
if (GetSslNegotiation(Settings) == SslNegotiation.Direct)
983+
{
984+
// We already check that in NpgsqlConnectionStringBuilder.PostProcessAndValidate, but since we also allow environment variables...
985+
if (Settings.SslMode is not SslMode.Require and not SslMode.VerifyCA and not SslMode.VerifyFull)
986+
throw new ArgumentException("SSL Mode has to be Require or higher to be used with direct SSL Negotiation");
987+
if (gssEncryptResult == GssEncryptionResult.NegotiateFailure)
988+
{
989+
// We can be here only if it's fallback from preferred (but failed) gss encryption
990+
// In this case, direct encryption isn't going to work anymore, so we throw a bogus exception to retry again without gss
991+
// Alternatively, we can instead just go with the usual route of writing SslRequest, ignoring direct ssl
992+
// But this is how libpq works
993+
Debug.Assert(gssEncryptionMode == GssEncryptionMode.Prefer);
994+
// The exception message doesn't matter since we're going to retry again
995+
throw new NpgsqlException();
996+
}
997+
998+
await DataSource.TransportSecurityHandler.NegotiateEncryption(async, this, sslMode, timeout, cancellationToken).ConfigureAwait(false);
999+
if (ReadBuffer.ReadBytesLeft > 0)
1000+
throw new NpgsqlException("Additional unencrypted data received after SSL negotiation - this should never happen, and may be an indication of a man-in-the-middle attack.");
1001+
}
1002+
else if ((sslMode is SslMode.Prefer && DataSource.TransportSecurityHandler.SupportEncryption) ||
1003+
sslMode is SslMode.Require or SslMode.VerifyCA or SslMode.VerifyFull)
1004+
{
1005+
WriteSslRequest();
1006+
await Flush(async, cancellationToken).ConfigureAwait(false);
1007+
1008+
await ReadBuffer.Ensure(1, async).ConfigureAwait(false);
1009+
var response = (char)ReadBuffer.ReadByte();
1010+
timeout.CheckAndApply(this);
1011+
1012+
switch (response)
1013+
{
1014+
default:
1015+
throw new NpgsqlException($"Received unknown response {response} for SSLRequest (expecting S or N)");
1016+
case 'N':
1017+
if (sslMode != SslMode.Prefer)
1018+
throw new NpgsqlException("SSL connection requested. No SSL enabled connection from this host is configured.");
1019+
break;
1020+
case 'S':
1021+
await DataSource.TransportSecurityHandler.NegotiateEncryption(async, this, sslMode, timeout, cancellationToken).ConfigureAwait(false);
1022+
break;
1023+
}
1024+
1025+
if (ReadBuffer.ReadBytesLeft > 0)
1026+
throw new NpgsqlException("Additional unencrypted data received after SSL negotiation - this should never happen, and may be an indication of a man-in-the-middle attack.");
1027+
}
1028+
}
1029+
10111030
async ValueTask<GssEncryptionResult> TryNegotiateGssEncryption(GssEncryptionMode gssEncryptionMode, bool async, CancellationToken cancellationToken)
10121031
{
10131032
// GetCredentialFailure is essentially a nop (since we didn't send anything over the wire)
10141033
// So we can proceed further as if gss encryption wasn't even attempted
10151034
if (gssEncryptionMode == GssEncryptionMode.Disable) return GssEncryptionResult.GetCredentialFailure;
10161035

1036+
// Same thing as above, though in this case user doesn't require GSS encryption but didn't enable encryption
1037+
// Most of the time they're using the default value, in which case also exit without throwing an error
1038+
if (gssEncryptionMode == GssEncryptionMode.Prefer && !DataSource.TransportSecurityHandler.SupportEncryption)
1039+
return GssEncryptionResult.GetCredentialFailure;
1040+
10171041
if (ConnectedEndPoint!.AddressFamily == AddressFamily.Unix)
10181042
{
10191043
if (gssEncryptionMode == GssEncryptionMode.Prefer)
@@ -1038,7 +1062,9 @@ static SslNegotiation GetSslNegotiation(NpgsqlConnectionStringBuilder settings)
10381062
return sslNegotiation;
10391063
}
10401064

1041-
return SslNegotiation.Postgres;
1065+
// If user hasn't provided the value via connection string or environment variable
1066+
// Retrieve the default value from property
1067+
return settings.SslNegotiation;
10421068
}
10431069

10441070
static GssEncryptionMode GetGssEncMode(NpgsqlConnectionStringBuilder settings)
@@ -1052,7 +1078,9 @@ static GssEncryptionMode GetGssEncMode(NpgsqlConnectionStringBuilder settings)
10521078
return gssEncMode;
10531079
}
10541080

1055-
return GssEncryptionMode.Disable;
1081+
// If user hasn't provided the value via connection string or environment variable
1082+
// Retrieve the default value from property
1083+
return settings.GssEncryptionMode;
10561084
}
10571085

10581086
internal async Task NegotiateEncryption(SslMode sslMode, NpgsqlTimeout timeout, bool async, CancellationToken cancellationToken)
@@ -2187,9 +2215,13 @@ void DoCancelRequest(int backendProcessId, int backendSecretKey)
21872215
{
21882216
try
21892217
{
2190-
RawOpen(Settings.SslMode, gssEncMode, new NpgsqlTimeout(TimeSpan.FromSeconds(ConnectionTimeout)), false,
2218+
var timeout = new NpgsqlTimeout(TimeSpan.FromSeconds(ConnectionTimeout));
2219+
RawOpen(timeout, false,
21912220
CancellationToken.None)
21922221
.GetAwaiter().GetResult();
2222+
SetupEncryption(Settings.SslMode, gssEncMode, timeout, false,
2223+
CancellationToken.None).
2224+
GetAwaiter().GetResult();
21932225
}
21942226
catch (Exception e) when (gssEncMode == GssEncryptionMode.Prefer)
21952227
{
@@ -2198,9 +2230,13 @@ void DoCancelRequest(int backendProcessId, int backendSecretKey)
21982230

21992231
// If we hit an error with gss encryption
22002232
// Retry again without it
2201-
RawOpen(Settings.SslMode, GssEncryptionMode.Disable, new NpgsqlTimeout(TimeSpan.FromSeconds(ConnectionTimeout)), false,
2233+
var timeout = new NpgsqlTimeout(TimeSpan.FromSeconds(ConnectionTimeout));
2234+
RawOpen(timeout, false,
22022235
CancellationToken.None)
22032236
.GetAwaiter().GetResult();
2237+
SetupEncryption(Settings.SslMode, GssEncryptionMode.Disable, timeout, false,
2238+
CancellationToken.None).
2239+
GetAwaiter().GetResult();
22042240
}
22052241

22062242
WriteCancelRequest(backendProcessId, backendSecretKey);

‎src/Npgsql/NpgsqlConnectionStringBuilder.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,7 @@ public SslNegotiation SslNegotiation
488488
[NpgsqlConnectionStringProperty]
489489
public GssEncryptionMode GssEncryptionMode
490490
{
491-
get => UserProvidedGssEncMode ?? GssEncryptionMode.Disable;
491+
get => UserProvidedGssEncMode ?? GssEncryptionMode.Prefer;
492492
set
493493
{
494494
UserProvidedGssEncMode = value;

‎src/Npgsql/Util/GSSStream.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public override void Write(ReadOnlySpan<byte> buffer)
5959
Unsafe.WriteUnaligned(ref _writeLengthBuffer[0], BitConverter.IsLittleEndian ? BinaryPrimitives.ReverseEndianness(written.Length) : written.Length);
6060

6161
_stream.Write(_writeLengthBuffer);
62-
_stream.Write(buffer.Slice(start, lengthToWrite));
62+
_stream.Write(_writeBuffer.WrittenMemory.Span);
6363

6464
_writeBuffer.ResetWrittenCount();
6565
start += lengthToWrite;

0 commit comments

Comments
 (0)