Apple Push Notification Service implementation - pitch

push
apns

#22

The Hardware Security Module itself performs the digital signature generation (so that the private key never leaves the module). Therefore, we would need to delegate the whole signing function.


(Kyle Browning) #23

Okay, so then we can attract the signature method to a protocol and provide a default of Signing Of File, Or Data, and then have a separate layer (outside of this implementation) that includes the hardware integration.

My concern is that not every server will have physical hardware so making that the default, or baking in AWS, alienates engineers not using AWS.

Unless im missing the concept of hardware security modules...


#24

Okay, so then we can attract the signature method to a protocol and provide a default of Signing Of File, Or Data

Sounds good to me!

and then have a separate layer (outside of this implementation) that includes the hardware integration

This layer would be provided by the developer of the application since its implementation can vary depending on the requirements. For example, the application may make a network call to a key management service or it may call a function from another library to communicate with a locally-installed Hardware Security Module.

My concern is that not every server will have physical hardware so making that the default, or baking in AWS, alienates engineers not using AWS.

My only thought was we should be able to delegate the digital signature generation to enable the usage of a key management service. We can still provide the default of generating the digital signature using a local private key as you said above.


(Kyle Browning) #25

Perfect, ill posit a PR and get your thoughts.


(Kyle Browning) #29

Alright, I've cleaned everything up here.

@fumoboy007^


(Kyle Browning) #30

Okay everything is incorporated now.

New usages for File Signer.

let apnsConfig = APNSConfig(keyId: "9UC9ZLQ8YW",
                            teamId: "ABBM6U9RM5",
                            signingMode: .file(path: "/Users/kylebrowning/Downloads/key.p8"),
                            topic: "com.grasscove.Fern",
                            env: .sandbox)

The new protocol APNSSigner allows you to make your own signer via whatever implementation you need

public class CustomSigner: APNSSigner {
   public func sign(digest: Data) throws -> Data {
     return try AWSKeyStore.sign(digest: digest)
   }
   public func verify(digest: Data, signature: Data) -> Bool {
      // verification
   }
}
let customSigner = CustomSigner()
let apnsConfig = APNSConfig(keyId: "9UC9ZLQ8YW",
                      teamId: "ABBM6U9RM5",
                      signingMode: .custom(signer: customSigner),
                      topic: "com.grasscove.Fern",
                      env: .sandbox)

(David Hart) #31

This is awesome! It's really great to have a top-quality APNS service on top of SwiftNIO. I haven't had time to look at the code in detail, but I had a few naming related suggestions that I compiled while I was reading through it if you're interested.

APNSConfig

  • I think getUrl would look even better as a computed url property.

APNSErrors

  • Cases are usually lower camel-cased. Is there a reason this enum breaks convention?

APNSSignatureError

  • Shouldn't ASN in invalidAsn1 be all caps?

Abbreviations

I have the impression that Swift code, especially in the Standard Library and Apple frameworks, tends to avoid shortened words and abbreviations at all cost. Here are a list of identifiers I wasn't expecting to see abbreviated:

  • APNSConfig (see URLSessionConfiguration)
  • APNSConfig.keyId (see NSBundle.bundleIdentifier)
  • APNSConfig.teamId
  • APNSConfig.env (see NSProcess.environment)
  • APNSConnection.connect.apnsConfig (in this case, I think apns doesn't add much - configuration would be sufficient IMHO).
  • APNSConnection.apnsConfig (same as above)
  • APNSEnv
  • APNSEnv.prod
  • APNSRequest.aps (I was actually very confused about this one. APS means Apple Push Service to me. Shouldn't this be called notification instead?)

(David Hart) #32

I forgot that that's the actual name of the property in the payload :-/ That's unfortunate.


(Kyle Browning) #33

Thanks for the review. Ill submit a PR that fits with Apple's standards, and take these into account. Once that's done ill reply here you to you and ask for a review again!

And yeah APNSRequest.aps is Apple's json payload. We could alternatively wrap this in a payload property that unpacks itself when building the request?


(David Hart) #34

What about a custom CodingKey?

public struct APNSRequest: Codable {
    enum CodingKeys: String, CodingKey {
        case payload = "aps"
        case custom
    }

    public let payload: Payload
    public let custom: [String: String]?

    public init(payload: Payload, custom: [String: String]? = nil) {
        self.payload = payload
        self.custom = custom
    }
}

I also made custom optional in the initialiser. That could be useful.

By the way, how does custom work if I want to output this example from the Apple documentation:

{
    "aps" : { "alert" : "Message received from Bob" },
    "acme2" : [ "bang",  "whiz" ]
}

(David Hart) #35

Although I'm not a fan of payload either because technically, the whole request is the payload.


(Kyle Browning) #36

As far as the custom portion. I wasn't able to think of a way to allow [String: Any]. Which is not Codable.

Any ideas?


(David Hart) #37

Not only the type seems a problem, but the current APNSRequest will encode custom into a "custom" JSON property, no? That was what worried me.

I've had an idea. How about a Notification protocol that must conform to Codable would allow users to encode whatever they want using Codable?

protocol Notification: Codable {
    var aps: APSPayload { get } // APSPayload == your APS
}

extension APNSConnection {
    // I switched arguments in send because it felt more natural. Thoughts?
    func send<T: Notification>(_ notification: T, to deviceToken: String) -> EventLoopFuture<APNSResponse> {
        // ...
    }
}

struct AcmeNotification: Notification {
    let acme2: [String]
    let aps: APSPayload

    init(acme2: [String], message: String) {
        self.acme2 = acme2
        aps = APSPayload(alert: Alert(message: message))
    }
}

let notification = AcmeNotification(acme2: ["bang",  "whiz"], message: "Message received from Bob")
let json = JSONEncoder().encode(notification)
// {
//    "aps" : { "alert" : "Message received from Bob" },
//    "acme2" : [ "bang",  "whiz" ]
// }

(Kyle Browning) #38

Okay will do! Thanks for the thoughts on this.

Feel free to submit a PR if I don't get to it by this Saturday morning.


(Kyle Browning) #39

I went ahead and did it all now before dinner haha.

I tested the custom options and they worked.

Heres the PR.


(David Hart) #40

That was fast! I'm glad you found the feedback useful. I left a few comments on the PR and explained what was wrong with the JSONEncoder.


(Kyle Browning) #41

Hey David, thanks for the feedback, Ive replied on the PR.

Seems the Swift still doesn't like that :frowning:


(Kyle Browning) #42

Thanks David merged!

Hope you don't mind the misnomer on your last name and commit message. I can amend it if you'd like, but I put David Hartbit haha.


(David Hart) #43

Nah, don't bother about it ^^


(Kyle Browning) #44

@IanPartridge have you, or anyone else you know started the JWT SSWG solution? I was going to work on separating that out in my spare time, but if work has started, id rather just help!