-
-
Notifications
You must be signed in to change notification settings - Fork 81
Webpush requests #1613
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
Webpush requests #1613
Conversation
23d86fe
to
2099a1a
Compare
2099a1a
to
9d30af4
Compare
logDebug "New WP Client requested" | ||
manager <- newManager tlsManagerSettings | ||
let c = wpPushProviderClient manager | ||
atomically $ TM.insert PPWebPush c pushClients |
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 don't think it'll work, even if the user can only use one push provider, all users can use different push providers, so the server should store in the map all of them, and either the key should be different or push provider should include endpoint (which is probably better, as endpoint is also part of token definition)
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.
so what would happen is that it would be pushing to a random push server
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.
It includes a single pusher but this pusher will request different URL based on the WPDeviceToken
wpPushProviderClient :: Manager -> PushProviderClient | ||
wpPushProviderClient mg tkn pn = do | ||
e <- endpoint tkn | ||
r <- liftPPWPError $ parseUrlThrow $ B.unpack e.endpoint |
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 parsing needs to happen when token is received, and converted to type that doesn't require parsing at a point of sending notification
wpEncrypt auth uaPubKS clearT = do | ||
salt :: B.ByteString <- liftIO $ getRandomBytes 16 | ||
asPrivK <- liftIO $ ECDH.generatePrivate $ ECC.getCurveByName ECC.SEC_p256r1 | ||
uaPubK <- point uaPubKS |
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 is effectively parsing, should be done in type parser, not here.
|
||
wpPushProviderClient :: Manager -> PushProviderClient | ||
wpPushProviderClient mg tkn pn = do | ||
e <- endpoint tkn |
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.
that also should not be failing
, requestBody = RequestBodyBS encBody | ||
, redirectCount = 0 | ||
} | ||
_ <- liftPPWPError $ httpNoBody req mg |
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 doesn't seem to use TLS
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.
see comment above
newWPPushClient :: NtfPushServer -> IO PushProviderClient | ||
newWPPushClient NtfPushServer {pushClients} = do | ||
logDebug "New WP Client requested" | ||
manager <- newManager tlsManagerSettings |
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.
need to use the same CA store that we use for APNS
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.
TODO: validate if it uses default system store, most likely it does not.
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.
It uses the default store:
ghci> man <- liftIO $ newManager tlsManagerSettings
ghci> let req = "https://perdu.com" { proxy = Nothing }
ghci> httpLbs req man
Response {responseStatus = Status {statusCode = 200, statusMessage = "OK"}, responseVersion = HTTP/1.1, responseHeaders = [("Date","Thu, 04 Sep 2025 09:10:39 GMT"),("Content-Type","text/html"),("Content-Length","163"),("Connection","keep-alive"),("last-modified","Thu, 02 Jun 2016 06:01:08 GMT"),("etag","\"cc-5344555136fe9-gzip\""),("accept-ranges","bytes"),("Cache-Control","max-age=600"),("expires","Thu, 04 Sep 2025 09:20:39 GMT"),("vary","Accept-Encoding,User-Agent"),("Content-Encoding","gzip"),("Server","cloudflare"),("Nel","{\"report_to\":\"cf-nel\",\"success_fraction\":0.0,\"max_age\":604800}"),("cf-cache-status","DYNAMIC"),("Report-To","{\"group\":\"cf-nel\",\"max_age\":604800,\"endpoints\":[{\"url\":\"https://a.nel.cloudflare.com/report/v4?s=IRxVE2a4P4b%2B9cRuL60q5q%2FHNbSTK7p%2FJcnjIl968IImjzem1ecVR9wcJyVY0bdIgpiFUAzHUvMXa0EJ%2FupmKHozM1qBUmD2jqFhUwdELqttVlE%3D\"}]}"),("CF-RAY","979c5ae08ed73e9e-MRS"),("alt-svc","h3=\":443\"; ma=86400")], responseBody = "<html><head><title>Vous Etes Perdu ?</title></head><body><h1>Perdu sur l'Internet ?</h1><h2>Pas de panique, on va vous aider</h2><strong><pre> * <----- vous êtes ici</pre></strong></body></html>\n", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose, responseOriginalRequest = Request {
host = "perdu.com"
port = 443
secure = True
requestHeaders = []
path = "/"
queryString = ""
method = "GET"
proxy = Nothing
rawBody = False
redirectCount = 10
responseTimeout = ResponseTimeoutDefault
requestVersion = HTTP/1.1
proxySecureMode = ProxySecureWithConnect
}
}
ghci> let req = "https://untrusted-root.badssl.com/" { proxy = Nothing }
<interactive>:31:5: warning: [GHC-63397] [-Wname-shadowing]
This binding for ‘req’ shadows the existing binding
defined at <interactive>:19:5
ghci> httpLbs req man
*** Exception: HttpExceptionRequest Request {
host = "untrusted-root.badssl.com"
port = 443
secure = True
requestHeaders = []
path = "/"
queryString = ""
method = "GET"
proxy = Nothing
rawBody = False
redirectCount = 10
responseTimeout = ResponseTimeoutDefault
requestVersion = HTTP/1.1
proxySecureMode = ProxySecureWithConnect
}
(InternalException (HandshakeFailed (Error_Protocol "certificate has unknown CA" UnknownCa)))
ghci>
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.
tlsManagerSettings loads certificates from /etc/ssl /system/etc/security/cacerts and /usr/local/share/certs ; if no CA are available, all requests fail
@@ -1382,7 +1382,8 @@ deleteCommand db cmdId = | |||
DB.execute db "DELETE FROM commands WHERE command_id = ?" (Only cmdId) | |||
|
|||
createNtfToken :: DB.Connection -> NtfToken -> IO () | |||
createNtfToken db NtfToken {deviceToken = DeviceToken provider token, ntfServer = srv@ProtocolServer {host, port}, ntfTokenId, ntfPubKey, ntfPrivKey, ntfDhKeys = (ntfDhPubKey, ntfDhPrivKey), ntfDhSecret, ntfTknStatus, ntfTknAction, ntfMode} = do | |||
createNtfToken db NtfToken {deviceToken, ntfServer = srv@ProtocolServer {host, port}, ntfTokenId, ntfPubKey, ntfPrivKey, ntfDhKeys = (ntfDhPubKey, ntfDhPrivKey), ntfDhSecret, ntfTknStatus, ntfTknAction, ntfMode} = do | |||
let (provider, token) = deviceTokenFields deviceToken |
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.
we most likely need to add fields for webpush tokens to the table.
| -- | unable to decode ecc key | ||
CryptoInvalidECCKey CE.CryptoError |
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.
| -- | unable to decode ecc key | |
CryptoInvalidECCKey CE.CryptoError |
This error is not necessary as it would be a parsing error.
data DeviceToken | ||
= APNSDeviceToken PushProvider ByteString | ||
| WPDeviceToken WPEndpoint |
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.
Proposed type for DeviceToken is
data DeviceToken
= APNSDeviceToken APNSProvider ByteString
| WPDeviceToken WPProvider WPTokenParams
newtype WPProvider = WPP (ProtocolServer 'WebPush) -- so we can add params if needed
data APNSProvider
= PPApnsDev -- provider for Apple development environment
| PPApnsProd -- production environment, including TestFlight
| PPApnsTest
data WPTokenParams = WPTokenParams
{ wpPath :: ByteString, -- parser should validate it's a valid type and possibly it should be
wpAuth :: ByteString, -- if we enforce size constraints, should also be in parser.
wpKey :: ECC.Point -- or another correct type that is needed for encryption, so it fails in parser and not there
}
General approach is to parse early, to the most narrow type that is applicable at the point of parsing, and not at the point of using it.
@@ -377,6 +378,7 @@ data PushProvider | |||
| PPApnsProd -- production environment, including TestFlight | |||
| PPApnsTest -- used for tests, to use APNS mock server | |||
| PPApnsNull -- used to test servers from the client - does not communicate with APNS | |||
| PPWebPush -- used for webpush (FCM, UnifiedPush, potentially desktop) | |||
deriving (Eq, Ord, Show) |
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.
Proposed type for push provider (it's needed as a key for the connected client), but it probably is not needed during parsing of DeviceToken:
data PushProvider = PPAPNS APNSProvider | PPWP WPProvider
pure $ APNSDeviceToken pp t | ||
|
||
-- | Returns fields for the device token (pushProvider, token) | ||
deviceTokenFields :: DeviceToken -> (PushProvider, ByteString) |
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 function seems unnecessary, as it's better to store components as separate fields and split/combine when saving to the database.
@@ -256,7 +257,8 @@ data APNSErrorResponse = APNSErrorResponse {reason :: Text} | |||
$(JQ.deriveFromJSON defaultJSON ''APNSErrorResponse) | |||
|
|||
apnsPushProviderClient :: APNSPushClient -> PushProviderClient | |||
apnsPushProviderClient c@APNSPushClient {nonceDrg, apnsCfg} tkn@NtfTknRec {token = DeviceToken _ tknStr} pn = do | |||
apnsPushProviderClient c@APNSPushClient {nonceDrg, apnsCfg} tkn@NtfTknRec {token} pn = do |
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 function needs to change to accept token components - APNSProvider and ByteString (token), so it does not fail on incorrect provider type. PPInvalidPusher error most likely is not necessary - it's not a real runtime error.
Added all (I think:) comments. I probably need to make a change on top with different types. |
replaced with #1642 |
-- | Elliptic-Curve-Point-to-Octet-String Conversion without compression | ||
-- | as required by RFC8291 | ||
-- | https://www.secg.org/sec1-v2.pdf#subsubsection.2.3.3 | ||
uncompressEncode :: ECC.Point -> BL.ByteString |
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.
uncompressEncode / uncompressDecode should be defined as Encoding (or StrEncoding if it's not a binary format) for some newtype, e.g.
newtype WPKey = WPKey ECC.Point
This type can then be used as part of the token, to parse earlier
encodeBigInt y | ||
uncompressEncode ECC.PointO = "\0" | ||
|
||
uncompressDecode :: BL.ByteString -> ExceptT CE.CryptoError IO ECC.Point |
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.
there is no reason for it to be IO, it needs to evaluate to Either
w64 :: Integer -> Bin.Word64 | ||
w64 = fromIntegral | ||
|
||
decodeBigInt :: BL.ByteString -> ExceptT CE.CryptoError IO Integer |
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.
there is no reason for it to be IO, it needs to evaluate to Either
where | ||
shift i w = Bits.shiftL (fromIntegral w) (64*i) | ||
|
||
encodePN :: PushNotification -> BL.ByteString |
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 looks like the usual JSON encoding, we should use generated ToJSON instance adding selectors to the type
hexStringP = | ||
A.takeWhile (`B.elem` "0123456789abcdef") >>= \s -> | ||
if even (B.length s) then pure s else fail "odd number of hex characters" | ||
|
||
instance ToJSON DeviceToken where | ||
toEncoding (DeviceToken pp t) = J.pairs $ "pushProvider" .= decodeLatin1 (strEncode pp) <> "token" .= decodeLatin1 t | ||
toJSON (DeviceToken pp t) = J.object ["pushProvider" .= decodeLatin1 (strEncode pp), "token" .= decodeLatin1 t] | ||
toEncoding token = case token of |
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.
why do we need JSON encodings for the token? If we do, then we need to use derived ToJSON/FromJson instances.
Possibly, it is only needed for webpush tokens?
This PR is based on #1612
It adds a WebPush push provider and do the web push requests
It doesn't contain the VAPID authorization yet (as the proposed implementation was using extras, that may be dropped)